-
Notifications
You must be signed in to change notification settings - Fork 361
Merge npm publishing jobs into a single file (Trusted Publishing - OICD) #2999
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
base: main
Are you sure you want to change the base?
Conversation
…pare for other Trusted Publisher changes
…can test by just starting the workflow. Also, change concurrency.
…completely serial with no cancellation!
…sing workflow_dispatch
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: 0 B Total Size: 497 kB ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (41c45a1) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2999If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR2999If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR2999 |
|
Verified that the snapshot publish works (it happened right on this PR!)
https://www.npmjs.com/package/@khanacademy/math-input/v/0.0.0-PR2999-20251029180740 And the Provenance reports the correct (new) workflow filename!
|
| label-name: item-splitting-change | ||
| comment-title: 🛠️ Item Splitting | ||
|
|
||
| publish_snapshot: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically copy-pasted over to publish.yml.
| @@ -1,96 +0,0 @@ | |||
| name: Release | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was all moved into publish.yml also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow only deals with publishing to Chromatic, so I've renamed the workflow file to be clearer (as we do Storybook stuff elsewhere too: gh-pages.yml, which publishes Storybook to our GitHub Pages environment).
| inputs: | ||
| run_type: | ||
| description: "The type of publish to test" | ||
| required: true | ||
| default: "snapshot" | ||
| type: choice | ||
| options: | ||
| - snapshot | ||
| - release | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding an input so that we can manually start either job in this file.
| concurrency: | ||
| group: ${{ github.workflow }} | ||
| # IMPORTANT! We do not want to cancel in-progress runs, because we don't | ||
| # want a production publish to be interrupted by a snapshot publish ever. | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires attention during review. I'm setting it so this workflow can only run one job at a time. This could mean snapshots take longer, but I think it's worth protecting.
Open to thoughts on other ways or things I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that this stuff is idempotent, so I don't think it matters if they are concurrent. Only one will win and the others will error out somehow.
I agree that we shouldn't cancel in progress though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly idempotent. However, if there were two pushes to a PR in close succession, the earlier one could complete after the latter resulting in the wrong code being marked as the "latest" version of that snapshot on npm.
Oh! If I add the branch, then we'd have safety for snapshot runs, but would be able to run a full release with any mount of snapshots safely and simultaneously.
I'll update that.
${{ github.workflow }}-${{ github.ref }}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this race was addressed in Wonder Blocks a while back by me. I don't remember what I did though.
.github/workflows/publish.yml
Outdated
| permissions: | ||
| id-token: write # Required for OIDC/Trusted Publishing and provenance | ||
| contents: write # For GitHub pages and creating release PRs | ||
| pull-requests: write # For snapshot PR comments | ||
| issues: write # For PR comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the union of permissions that the release and snapshot jobs had in the other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the GitHub docs to verify what you need. At the job level, permissions are a complete overwrite, so if you want to add permissions, you also need to list the default permissions.
I don't know how workflow-level permissions integrate with job level. I would assume they are additive to avoid needing to explicitly list defaults, but worth checking to be sure.
| if: | | ||
| (github.event_name == 'workflow_dispatch' && inputs.run_type == 'release') || | ||
| (github.event_name == 'push' && github.ref == 'refs/heads/main') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the original if but adds workflow_dispatch and checking the input param to see if we're asked to run this job.
.github/workflows/publish.yml
Outdated
| id-token: write # required for publishing to npm with provenance | ||
| contents: write # required for creating pull requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't think this is needed given we define permissions at the file-level above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is. I would remove it. You'll find out the first time you use this and it'll be easy to fix.
| (github.event_name == 'workflow_dispatch' && inputs.run_type == 'snapshot') || | ||
| (github.event_name == 'pull_request' && !startsWith(github.head_ref, 'changeset-release/')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the original if but adds workflow_dispatch and checking the input param to see if we're asked to run this job.
.github/workflows/publish.yml
Outdated
| permissions: | ||
| id-token: write # required for publishing to npm with provenance | ||
| pull-requests: write # required because we write a comment on the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think I can remove this now that we have permissions defined at the file level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will run on your PR, why not remove it and try it. Easy to fix if it's needed.
somewhatabstract
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Some inline comments but nothing huge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Thanks for making this clarifying change!
| concurrency: | ||
| group: ${{ github.workflow }} | ||
| # IMPORTANT! We do not want to cancel in-progress runs, because we don't | ||
| # want a production publish to be interrupted by a snapshot publish ever. | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that this stuff is idempotent, so I don't think it matters if they are concurrent. Only one will win and the others will error out somehow.
I agree that we shouldn't cancel in progress though.
.github/workflows/publish.yml
Outdated
| permissions: | ||
| id-token: write # Required for OIDC/Trusted Publishing and provenance | ||
| contents: write # For GitHub pages and creating release PRs | ||
| pull-requests: write # For snapshot PR comments | ||
| issues: write # For PR comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the GitHub docs to verify what you need. At the job level, permissions are a complete overwrite, so if you want to add permissions, you also need to list the default permissions.
I don't know how workflow-level permissions integrate with job level. I would assume they are additive to avoid needing to explicitly list defaults, but worth checking to be sure.
.github/workflows/publish.yml
Outdated
| id-token: write # required for publishing to npm with provenance | ||
| contents: write # required for creating pull requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is. I would remove it. You'll find out the first time you use this and it'll be easy to fix.
.github/workflows/publish.yml
Outdated
| permissions: | ||
| id-token: write # required for publishing to npm with provenance | ||
| pull-requests: write # required because we write a comment on the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will run on your PR, why not remove it and try it. Easy to fix if it's needed.
…of having workflow-level permissions
…NPM_TOKEN" This reverts commit ed20bfc.


Summary:
This PR co-locates the two jobs that publish to npm (release and snapshot). This is a necessity for enabling Trusted Publishing (https://docs.npmjs.com/trusted-publishers#how-trusted-publishing-works).
Once this PR has landed, we'll be able to go into each npm package's settings on npmjs.com and enable Trusted Publishing. Once that's enabled, we no longer need a npm token (stored in
NPM_TOKEN) in this repo at all and will no longer have to manage an auth token for publishing to npm.Issue: LEMS-3681
Test plan:
😅 This will be tricky.
NPM_TOKENand remove that secret from this repo.