Conversation
|
There was a problem hiding this comment.
I don't see how this meaningfully protects anything, since "publishing" in this context means pushing to a branch/tag/whatever. I suspect if we want to do any sort of protecting things, it would involve branch protections. Like CODEOWNERS here does nothing, CODEOWNERS + branch protection restricting by CODEOWNERS can certainly do something but i don't think this does. I think the solution might involve the release script creating another PR to the branch with the built version after merging in the Version packages which would be akin to the environment approval and the merge would require approval or something by myself/you/whoever because of a branch protection on the release. (probably use the newer iteration of branch protections "Rulesets" to do it)
For preview "releasing" PRs, maybe we want a job which would run the build and then push to branch like maybe ${branchName}-built or something but for forks, it would do that on the forked repo, not this repo. Maybe this is just a workflow dispatch which accepts what branch it should "release" and a contributor could run it on their fork if they want?
|
I think the current improved setup with branch+tag ruleset protections, required env approvals and bot having bypass on those protections works quite OK and doesn't require complicating the workflow further. WDYT? As to preview releasing, we could use a workflow dispatch, sure. I feel the comment is slightly nicer as it doesn't require switching tabs and stuff and creates a nice "log" of trigger->failure/success messages in the comments. This is also something that is going to be rarely used by us really and ultimately, I don't think it requires extra protections as this is only a preview channel and not a prod one (and only collaborators can trigger those previews too) |
| * @Andarist @emmatown | ||
| /.github/workflows/** @Andarist @emmatown | ||
| /action.yml @Andarist @emmatown | ||
| /scripts/bump.ts @Andarist @emmatown | ||
| /scripts/release.ts @Andarist @emmatown | ||
| /scripts/release-pr.ts @Andarist @emmatown |
There was a problem hiding this comment.
I'm not sure what value the CODEOWNERS has?
There was a problem hiding this comment.
Not a lot really - it's just an extra precaution so we are always required to review those particular changes
|
|
||
| - name: Create or update release pull request | ||
| id: changesets | ||
| uses: ./ |
emmatown
left a comment
There was a problem hiding this comment.
Just want to note that from reading the changes, I expect that the release PR workflow wouldn't work on PRs from forks now but otherwise, looks good
No description provided.