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

ci: provenance build and publish #118

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Aug 28, 2024

  • github ci: check for consistent git dir after build and test
  • github ci: add publish-npm workflow

Related

@legobeat legobeat marked this pull request as ready for review August 28, 2024 22:31
@paulmillr
Copy link
Collaborator

Why is consistent git check useful?

@legobeat
Copy link
Contributor Author

legobeat commented Aug 29, 2024

Why is consistent git check useful?

I personally find it helpful to ensure there are no unintended side-effects or new artifacts as part of in particular build but also tests. While npm ci does ensure lockfile consistency already, I think a second layer doesn't hurt when it's unintrusive like this.
Let me know if you prefer either of both of those checks removed, though!

@paulmillr
Copy link
Collaborator

Let’s remove git check for 2 reasons:

  1. I try to keep workflow files same across all my repos for easy updates
  2. It’s nice check, but it’s more useful to do it on developer machine instead of server ci. So that dev machine doesn’t commit shit to repo. And workflow files aren’t being ran on dev machines

@paulmillr
Copy link
Collaborator

And of course reason number 3:

Too many checks degrade devEx terribly.

I’ve once tried contributing to some old eth ecosystem pkg. it had all sorts of checks: pre commit hook, linters and others.

at some point I was not able to commit to the repo — it was too complex. So, for non important things, the less, the better.

@legobeat legobeat marked this pull request as ready for review August 29, 2024 19:19
@legobeat
Copy link
Contributor Author

legobeat commented Aug 29, 2024

Removed the checks!

I can very much sympathize with point especially point 3.. Though I'd still recommend checking to catch such discrepancies in CI (like a new [`*.js`](https://github.com/ethereum/js-ethereum-cryptography/blob/2f7e6f303c4848970e9fabfcab4a0ecb0bd2a88f/package.json#L13) or `*.d.ts` file inadvertently being created or changed in such a way as you describe down the line...) at least for publish - and therefore for build - but perhaps for another day, then!

@paulmillr paulmillr merged commit 7ae8df8 into ethereum:master Aug 29, 2024
1 check passed
@paulmillr
Copy link
Collaborator

no we need to find someone with root repo access who can add npm keys to ENV vars

@legobeat legobeat deleted the github-publish branch August 30, 2024 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants