Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update nix flake impl #725

Merged

Conversation

andrew-scott-fischer
Copy link
Contributor

This commit removes npm based pre-commit hooks in favor of pre-commit-hooks managed by Nix.

@andrew-scott-fischer andrew-scott-fischer requested a review from a team as a code owner March 30, 2024 00:03
Copy link

socket-security bot commented Mar 30, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@andrew-scott-fischer andrew-scott-fischer marked this pull request as draft March 30, 2024 00:06
@andrew-scott-fischer andrew-scott-fischer force-pushed the update-nix-flake-impl branch 2 times, most recently from 541a14c to a8f69cb Compare April 1, 2024 15:47
@andrew-scott-fischer andrew-scott-fischer marked this pull request as ready for review April 1, 2024 16:32
.nvmrc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file. There's no point having it if we're not using it in .ci, and we shouldn't be, because we want actions/setup-node to use the latest LTSes dynamically

Copy link
Contributor Author

@andrew-scott-fischer andrew-scott-fischer Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt like this would enable someone who doesn't use nix to contribute, or at least build things locally. It also acts as a short hand to see which version of node nix is installing from the nixpkgs, since that's not outlined in the flake.nix.

Are you sure you prefer it to be removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt like this would enable someone who doesn't use nix to contribute, or at least build things locally.

We should be ambivalent to which LTS version of Node.js a contributor uses, so this is not a large factor in my opinion.

It also acts as a short hand to see which version of node nix is installing from the nixpkgs, since that's not outlined in the flake.nix.

We can simply run node --version to see what version of node Nix is installing, this isn't worth committing another file to our repository, especially one that is a derivation of other committed files.

Are you sure you prefer it to be removed?

Yes please

flake.nix Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Nix is the standard interface through which BitGo configures
pre-commit-hooks. Removing the npm based pre-commit hooks is the first
stage in replacing them with Nix based hooks.
.nvmrc Outdated Show resolved Hide resolved
Overhauls the nix flake to no longer use flake-utils , adds
pre-commit-hooks, and writes the currently used `node --version` to
an `.nvmrc`
Bring all files under `prettier`'s all-seeing eye
Copy link
Contributor

@ericcrosson-bitgo ericcrosson-bitgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaces pre-commit and lint-staged with pre-commit-hooks.nix ❄️ 🚀

Thanks @andrew-scott-fischer!

@ericcrosson-bitgo ericcrosson-bitgo merged commit 431010b into BitGo:master Apr 1, 2024
6 checks passed
Copy link

github-actions bot commented Apr 1, 2024

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants