-
Notifications
You must be signed in to change notification settings - Fork 38
ci: Set up OIDC NPM publishing (initial attempt) #768
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
Conversation
|
Random question while I am omw home to review proper: couldn't we use composite actions for the release flows? That is, we define the steps as composite actions and then based on input run the pre release or non pre release flow 👀 Not sure if it even works but 🤷♂️ thought I'd throw that into the bowl |
| elif [[ "${{ github.event_name }}" == "push" ]] && \ | ||
| [[ ! "${{ github.event.head_commit.message }}" =~ ^docs ]] && \ | ||
| [[ ! "${{ github.event.head_commit.message }}" =~ ^ci ]] && \ | ||
| [[ "${{ github.repository }}" =~ ^apify/ ]]; then |
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 would move the checks for repository out into each job instead
Coincidentally, I just finished this monstrosity: apify/apify-docs#2032 Which goes in the very same direction, a lot of duplicity, but also a lot of "simplicity". |
As long as there is only one workflow that calls |
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.
looks good to me, not great, not terrible. i'll surely prefer readability over deduplication.
sidenote: i am quite sure that after we finish migration in the last repo, they will allow whitelisting multiple workflows ![]()
Same here. Alternative 2 (a custom action to trigger a publishing workflow) is the only one that I'd consider more seriously. @barjin and @vladfrangu, are you OK with that?
Right? 😁 |
|
Considering how many repos we will need to change, I would keep things simple. |
I'm 100% on board with that. The custom action would only have to be written once and it would probably help make diffs smaller during the migration. |
|
That is what I was kinda hinting at with reusable actions -> we could move our release flows (if possible?) to /workflows and re-use them across repos... But I am not as up to date with our release pipeline to know if thats feasible enough :D |
|
I'll leave this up to you guys, if we can make it simpler than this without much effort, I'm all in for that, but if it would mean more than a day of work, it doesn't seem worth it. |
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.
Thank you @janbuchar, this looks good to me.
The current solution is imo perfectly sufficient, I agree that deduplicating this would likely result in hard-to-read bash/yaml ternary exprs.
| # ROUTE DECISION | ||
| # ============================================================================ | ||
|
|
||
| determine_path: |
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.
nit: maybe (determine|pick)_release_type would fit this job better. At first, I thought "release path" would be a filesystem path with the build.
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.
yeah, and i also like pick better than determine
|
After an internal discussion, we decided to go with the alternative approach (custom action that triggers a separate workflow). |
This is the first one in a series of PRs to set up OIDC based publishing to npm. It deserves special attention because it will shape the way how we approach this for the rest of the packages.
The Challenge
npm only allows configuring one workflow as the trusted publisher. That's it. But our repositories currently use two workflow files, one for pre-releases and one for stable ones.
Selected solution
I simply merged the two workflows in a single file, almost as if they were still separate. Both branches depend on a
determine_pathjob that analyzes the event that triggered the workflow. From that point, the execution paths are completely disjoint.This has a major advantage - both the review of the changes and of the workflow are fairly straightforward. Experience suggests that attempting to "DRY this up" makes the resulting workflow shorter, but very hard to follow.
Alternatives