Skip to content

Conversation

@Totto16
Copy link
Collaborator

@Totto16 Totto16 commented Mar 24, 2025

Add a release CI

This runs on every prerelease and release, and published the package automatically to npm, so we don't have to do that manually.

If it is a pre-release, it is published under the next tag, otherwise under the latest tag.

This was not tested as a whole yet (as it is not possible to test everything together that easily), but it should work, once
all TODO's are done. I tested every step of the pipeline separately locally, the pipeline was run nearly identical already on a test repo of mine (without npm publish)

Additionally I updated the normal Ci, to also use the newest Node LTS version and cleaned it up a bit.

Links

What is Provenance: https://docs.npmjs.com/generating-provenance-statements
GH example for publishing packages: https://docs.github.com/de/actions/use-cases-and-examples/publishing-packages/publishing-nodejs-packages

TODO

@Totto16 Totto16 requested a review from JumpLink March 24, 2025 21:45
add `id-token` write permissions, so that provenacne works correctly
Copy link
Collaborator

@swsnr swsnr left a comment

Choose a reason for hiding this comment

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

I think this is a good idea, but since this is a shared repository we need a bit of security in place to avoid someone stealing the token for releases. In particular, we might want to provide collaborators with write access, while still not permitting them to do releases.

Specifically, I think we need to

  • make sure that the NPM_TOKEN is only available to workflows on tags and the main branch,
  • in other words, make sure that the token is never available in other branches or pull requests, even from this repo, and
  • setup CODEOWNERS to enforce review and approval for all changes to workflows by repo owners (and not just repo collaborators).

I don't actually know how to set that up with Github actions, but I think it's mandatory to have this secured.

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 25, 2025

I think this is a good idea, but since this is a shared repository we need a bit of security in place to avoid someone stealing the token for releases. In particular, we might want to provide collaborators with write access, while still not permitting them to do releases.

Specifically, I think we need to

* make sure that the NPM_TOKEN is only available to workflows on tags and the main branch,

* in other words, make sure that the token is never available in other branches or pull requests, even from this repo, and

* setup CODEOWNERS to enforce review and approval for all changes to workflows by repo owners (and not just repo collaborators).

I don't actually know how to set that up with Github actions, but I think it's mandatory to have this secured.

I agree in general with your view, but GH actions and secrets are safe, and can't be retrieved in any way.

Edit: I get the exploit that might reveal the secret (creating a PR, that changes the CI, to echo the secrets, it runs automatically and prints the secret)

However, making the token only available on certain branches (e.g. main ) should be possible. There is good documentation for that, AFAIK we need to just make a branch rule / ruleset, where we add the secret just for those branches.

I don't think setting up CODEOWNERS is necessary and there is only the option, that only CODEOWNERS can approve PR's, there is no scope, that restricts chnages in workflows.

@swsnr
Copy link
Collaborator

swsnr commented Mar 25, 2025

In the code owners file you declare that only e.g. repo owners are code owners of workflows files, and then Github requires a review from these people.

I think that this is mandatory to prevent unauthorised modifications to workflows.

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 25, 2025

In the code owners file you declare that only e.g. repo owners are code owners of workflows files, and then Github requires a review from these people.

I think that this is mandatory to prevent unauthorised modifications to workflows.

I never used that feature, so I am not familiar with that. But if it's possible, It's a good idea. 👍🏼

@JumpLink
Copy link
Collaborator

JumpLink commented Mar 25, 2025

Thanks a lot and great idea! I'll have a look at this over the next few days and enter the NPM token in a secure way and then give appropriate feedback 👍

- Require review from core maintainers for critical files
- Protect workflow files, package.json, and release configurations
- Ensure proper oversight for npm publishing pipeline
- Remove redundant release.yml rule (covered by /.github/)
- Add specific rule for CODEOWNERS file itself
- Simplify and improve maintainability
- Enable shared responsibility for access control management
- All core maintainers can now propose and approve CODEOWNERS changes
- Improves team autonomy and reduces single point of failure
- Focus protection on critical infrastructure files only
- Allow normal code contributions without mandatory maintainer review
- Make protected files more explicit and clear
- Reduce friction for regular contributors
- Add npm-release environment to release workflow
- Simplify build job name for better status check integration
- Enable environment-specific NPM_TOKEN access
JumpLink
JumpLink previously approved these changes Aug 16, 2025
Copy link
Collaborator

@JumpLink JumpLink left a comment

Choose a reason for hiding this comment

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

Hi @Totto16 and @swsnr

Sorry for the long delay on this excellent PR! 🙏 It unfortunately got buried in my backlog.

I've now added the security configurations we discussed:

CODEOWNERS file - protects critical infrastructure files
NPM_TOKEN in protected environment (npm-release)
Branch protection rules - requires reviews from maintainers
Updated workflows for environment integration

All the TODOs are hopefully completed.

Thanks for your patience, ready to merge when you're happy with the setup.

- Align with existing package.json publish scripts
- Remove separate pack step, yarn handles packaging automatically
- Maintain provenance and tag logic
@JumpLink JumpLink force-pushed the add_release_ci_npm_release branch from 3b91389 to 694ea66 Compare August 16, 2025 07:08
@JumpLink
Copy link
Collaborator

@Totto16 I have adjusted the release script so that yarn is used for this. I hope it continues to work; otherwise, we can change it back to pure npm.

@JumpLink JumpLink self-requested a review August 16, 2025 07:09
JumpLink
JumpLink previously approved these changes Aug 16, 2025
swsnr
swsnr previously approved these changes Aug 16, 2025
@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

@Totto16 I have adjusted the release script so that yarn is used for this. I hope it continues to work; otherwise, we can change it back to pure npm.

I think as yarnpkg/berry#5430 is fixed, that yarn should work too, I'll test it locally again, then I'll merge it 👍🏼

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

This doesn't work with the current yarn version (4.7.0) as this feature for yarn was only published in 4.9.0 (see yarnpkg/berry#6750) So I updated to yarn 4.9.2 and it works locally. 😄

@Totto16 Totto16 dismissed stale reviews from swsnr and JumpLink via 426a88d August 16, 2025 11:34
@Totto16 Totto16 force-pushed the add_release_ci_npm_release branch from 426a88d to cbb79c3 Compare August 16, 2025 11:35
@JumpLink JumpLink self-requested a review August 16, 2025 11:42
Copy link
Collaborator

@JumpLink JumpLink left a comment

Choose a reason for hiding this comment

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

@Totto16 nice 👍

@Totto16 Totto16 merged commit c714c82 into main Aug 16, 2025
2 checks passed
@Totto16 Totto16 deleted the add_release_ci_npm_release branch August 16, 2025 11:44
@JumpLink
Copy link
Collaborator

@Totto16 Feel free to create a new release to test this. With #70, there are also some new changes.

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

@Totto16 Feel free to create a new release to test this. With #70, there are also some new changes.

I already checked, if I could do that, but I might have used the wrong compare order 😓

main...48.0.2

vs

48.0.2...main

🤦🏼‍♂️

I'll do that, lets see if it works 👀

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

@JumpLink I think we "broke" something, as for releases we always created a separate commit like e.g. 0723ca5

But I can't push any commit to main 😓 as only PR's are allowed to push to it, so either we make PR's just for such version bump or we allow it in some other way (like e.g. force pushes, I am not a fan of them, but as most if not all try normal pushes and only use force, when that doesn't work it might be ok 🤷🏼‍♂️ )

image (sorry for some german git stuff, but I think the core part is understandable)

@JumpLink
Copy link
Collaborator

JumpLink commented Aug 16, 2025

@Totto16

How about automatically setting the version number via GitHub action and pushing it when a release is made? The version number could be extracted from the Git tag, which is specified anyway when a release is made. I think yarn can also set the version numbers of the packages.

But I don't know if it's possible to push anything else in a release process.

But let's test it first to see if it works without it. I think it's okay to create a PR even for a version jump, but you can also change the protection, now.

(P.S. I'm also German, like many of us, coincidentally ^^')

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

@Totto16

How about automatically setting the version number via GitHub action and pushing it when a release is made? The version number could be extracted from the Git tag, which is specified anyway when a release is made. I think yarn can also set the version numbers of the packages.

But I don't know if it's possible to push anything else in a release process.

But let's test it first to see if it works without it. I think it's okay to create a PR even for a version jump, but you can also change the protection, now.

I think setting the version number in the package.json files on releases is possible, since you can push commits in GH actions, the problem would be, we need to write a script of some sort, that auto detects normal versions vs. pre-releases. And the Ci also would need to push to main or make an PR.

That the CI runs because of the release "hook" doesn't matter, there is no special thing, that prevents you from pushing commits, BUT since the tag already exists, and points to a commit, where the version is the old one, it would need to detect the version of that tag, change the versions in the package.json files, delete the upstream tag, make a new one that points to the newly created tag and push these things to the repo.

I think that is too complicated and also not worth, as the script to detect pre-releases would be complicated IMO.

A PR for version bumps is also not that bad of an idea, as than at least one maintainer needs to approve the release 👍🏼

(P.S. I'm also German, like many of us, coincidentally ^^')

Oh, I never noticed, so now we can write all comments in German 😄 (Just kidding xD)

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

@JumpLink that didn't work that well 😂

https://github.com/gjsify/gnome-shell/actions/runs/17009833794

image

@JumpLink
Copy link
Collaborator

@Totto16 See if you can adjust the settings, I can't right now

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

@Totto16 See if you can adjust the settings, I can't right now

I already did and ran in another problem 😓

I allowed all tags, atm, but we need to see if this is a good idea 🤔

I'll just post updates here, so that everyone sees the process, I'll try to take care of the problem myself 😄

@Totto16
Copy link
Collaborator Author

Totto16 commented Aug 16, 2025

I tried a few things, but the yarn workspace @girs/gnome-shell seems to break it, as before that change it worked, the setup to test this is a little complex, so I tried to test this in this repo, by deleting the release, making a commit diretcly to main and making the release again, but as the problem is more complex, I need to test this again in the personal setup, so that it doesn't require pushes to main branch, re-adding releases etc. (and also release pings to the users 😓 sorry about that)

So I'll need to fix this in a separate PR.

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.

4 participants