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

chore: upgrade aegir, switch to unified ci #142

Merged
merged 5 commits into from
Apr 14, 2022

Conversation

achingbrain
Copy link
Collaborator

Updates project config to add autorelease in the unified CI model as requested in this comment - #136 (comment)

Before merging this:

  1. Check npm config of https://www.npmjs.com/package/@chainsafe/libp2p-noise - "Publishing access" should be set to "Require two-factor authentication or automation tokens"
  2. Create a repository secret here called NPM_TOKEN with a npm automation token that can publish to @chainsafe/libp2p-noise.
  3. Add https://github.com/web3-bot as a collaborator to this repo to receive automated CI config updates

Updates project config to add autorelease in the [unified CI model](https://github.com/protocol/.github) as requested in this comment - ChainSafe#136 (comment)

Before merging this:

1. Check npm config of https://www.npmjs.com/package/@chainsafe/libp2p-noise - "Publishing access" should be set to "Require two-factor authentication or automation tokens"
2. Create a repository secret here called `NPM_TOKEN` with a npm automation token that can publish to `@chainsafe/libp2p-noise`.
3. Add https://github.com/web3-bot as a collaborator to this repo to receive automated CI config updates
@achingbrain
Copy link
Collaborator Author

Yarn.

So, for some reason, [email protected] consistently symlinks the wrong tsc binary in node_modules/.bin.

The dep tree is:

@chainsafe/[email protected] /Users/alex/Documents/Workspaces/ChainSafe/js-libp2p-noise
└─┬ [email protected]
.. other deps
  ├─┬ [email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └── [email protected]
.. other deps
 └── [email protected]

It finds [email protected] as a dep of detective-typescript instead of [email protected] as a dep of aegir.

This is a problem because [email protected] doesn't support the right values for the lib option and compiling the typescript code fails.

One solution might be to add a redundant typescript dep to this module and hope that the dependency resolution algorithm would choose that one to symlink, though it's a bad solution because it's not used directly here, plus you'd then need to keep it in sync with the aegir version which is extra work no-one needs.

Unified CI uses npm and not yarn, so I've removed yarn.lock since it's redundant from a CI point of view but there's nothing to stop people using yarn if they want to, though at the moment it seems to have a bug in it.

@achingbrain achingbrain changed the title chore: switch to unified ci chore: upgrade aegir, switch to unified ci Apr 11, 2022
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

FYI I had to make one feature release that was forgotten so we don't screw up versioning when we merge this.

I've added web3-bot and added NPM_TOKEN so we are basically ready.

That said, I don't think this is working 😅 , as ci is using release script which is missing along with semantic release configuration.
But before you fix that, if I'm not mistaken we would need to remove the branch protection rule as well so that CI can commit changelog and package.json?

I'm not really a huge fan of this especially considering it's kinda against our company policy to leave unprotected branch like that but more importantly, GitHub's actions had a lot of exploits last year so it's even more dangerous.

I would propose that we implement a release process more similar to this (and js-libp2p repo as well): https://github.com/ChainSafe/strip-comments/tree/master/.github/workflows
I would be happy to modify this PR.

It's using semantic PRs with squash merge and release-please action.
@wemeetagain @dapplion @tuyennhv could you chime in as well.

@achingbrain if we are running slightly different workflows would we still get PR from web3-bot with CI updates?

@dapplion
Copy link
Contributor

@mpetrunic That looks like a good middle ground solution. Definitely better than current status quo. We can go for this approach and switch to @achingbrain approach if it's doesn't provide good enough DX.

Lodestar's security is that of the weakest link so since we are already dependant on many js-ipfs libraries, using the auto-release strategy does not reduce the security of those packages. Consumers usually depend on noise + libp2p, but not noise in isolation right? It would have a greater impact that we all do the switch tho

@achingbrain achingbrain requested a review from a team as a code owner April 13, 2022 10:40
@mpetrunic
Copy link
Member

@dapplion @wemeetagain pushed changes.

@achingbrain
Copy link
Collaborator Author

But before you fix that, if I'm not mistaken we would need to remove the branch protection rule as well so that CI can commit changelog and package.json?

You can usually exclude certain users from protection rules in these sort of setups? There's a possible solution here. Do we want that though? It sounds like maybe not.

If we are running slightly different workflows would we still get PR from web3-bot with CI updates?

web3-bot will automatically overwrite any files in .github/workflows with the same names as https://github.com/protocol/.github/tree/master/templates/.github/workflows when they change in that repo.

That said, libp2p and ipfs both use release-please as a release gate rather than semantic-release on every commit, we could add a yml file for that workflow to https://github.com/protocol/.github/tree/master/templates/.github/workflows and use that here as well?

I think then you can skip the CI-user-exclusion-from-protected-branches mentioned above.

Though the joy of semantic-release is you don't have to bug people to merge release PRs, which was the original ask.

Happy to do whatever you prefer though.

@mpetrunic
Copy link
Member

web3-bot will automatically overwrite any files in .github/workflows with the same names as https://github.com/protocol/.github/tree/master/templates/.github/workflows when they change in that repo.

That said, libp2p and ipfs both use release-please as a release gate rather than semantic-release on every commit, we could add a yml file for that workflow to https://github.com/protocol/.github/tree/master/templates/.github/workflows and use that here as well?

Seems like my changes pushed here are compatible then.

Though the joy of semantic-release is you don't have to bug people to merge release PRs, which was the original ask.

Happy to do whatever you prefer though.

I think our biggest issue was that nobody wanted to create release PR with changelog and version updates, merging it is not that much of an issue. This also doesn't force people (external contributors) to use semantic commits if they don't want to and we can influence version bump by editing PR title.

@mpetrunic mpetrunic merged commit 114e437 into ChainSafe:master Apr 14, 2022
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.

3 participants