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

docs: add instructions about applying rulesets #30

Merged
merged 6 commits into from
Apr 2, 2024
Merged

Conversation

tjsilver
Copy link
Contributor

@tjsilver tjsilver commented Mar 13, 2024

What does this change?

Adds some instructions on how to apply rulesets for branch protection and status checks for Guardian developers, once they have disabled branch protection when applying this workflow to a repo.

This allows repos to keep the same level of protection as before, but allows for the Scala release app to bypass and run successfully.

Update: the instructions now link out to a page on the guardian/recommendations repo.

@tjsilver tjsilver requested review from rtyley and a team March 13, 2024 15:13
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
@rtyley
Copy link
Member

rtyley commented Mar 14, 2024

Thanks for this! I do have a few points:

  • I would like to make this workflow and its documentation usable by other organisations, not just the Guardian, so having large amounts of Guardian-specific stuff in the middle of the general Configuration document is quite intrusive. For ease of Guardian developers using the documentation, I'm certainly happy with the occasional single line giving Guardian-specific information, but for more extensive instructions, like these, I think it would be better to link off to a Guardian-specific document elsewhere?
  • The general instructions about setting up branch Rulesets (push protection and CI requirement) are likely to apply to many other Guardian repos (Scala apps, Typescript apps & libraries, etc), not just Scala library repos, so they probably don't belong in gha-scala-library-release-workflow, but should be well-documented elsewhere! The bypass exemption for the gu-scala-library-release bot definitely does need to be documented, and that could live with that main Ruleset documentation, and be referenced with a pointer in the docs in gha-scala-library-release-workflow
  • I was a bit surprised we're asking devs to manually setup a CI Ruleset on each repo - that feels like something RepoCop might be able to do? I haven't been part of the discussion on RepoCop's role on that, I probably missed something!

@NovemberTang
Copy link
Contributor

NovemberTang commented Mar 15, 2024

  • I was a bit surprised we're asking devs to manually setup a CI Ruleset on each repo - that feels like something RepoCop might be able to do? I haven't been part of the discussion on RepoCop's role on that, I probably missed something!

We (me and @tjsilver ) tried this out a few days ago to no avail. To set up a status/CI check on a ruleset you need to pass in the exact name of the workflow(s). Most of the time, this is just CI, but this isn't true enough of the time for us to be able to generalise. If they have a second workflow called test or migrate or anything else, it won't be run, and you'll have to set up a custom ruleset for the repo to catch that anyway. Conversely, if the repo doesn't have a status check called CI - and we've specified that we want it to look for one - it'll hang forever.

I'm hoping that GitHub improves this story over time, or that maybe there's something I've missed, but based on what I've found so far, I don't think it's possible at the moment.

@rtyley
Copy link
Member

rtyley commented Mar 15, 2024

Most of the time, this is just CI, but this isn't true enough of the time for us to be able to generalise. If they have a second workflow called test or migrate or anything else, it won't be run, and you'll have to set up a custom ruleset for the repo to catch that anyway. Conversely, if the repo doesn't have a status check called CI - and we've specified that we want it to look for one - it'll hang forever.

Yeah, that makes sense - I guess to an extent standardisation would help here - like, maybe teams should just standardise on having a CI workflow called 'CI'? RepoCop could check for the presence of a 'CI' workflow, and if one exists, set the ruleset up, and if it doesn't, perhaps warn teams that it would be good to standardise on 'CI' - though there may be legitimate cases where complex projects have multiple CI jobs and there is no obvious single test to look at.

@tjsilver
Copy link
Contributor Author

* I would _like_ to make this workflow and its documentation usable by other organisations, not just the Guardian, so having large amounts of Guardian-specific stuff in the middle of the general Configuration document is quite intrusive. For ease of Guardian developers using the documentation, I'm certainly happy with the occasional single line giving Guardian-specific information, but for more extensive instructions, like these, I think it would be better to link off to a Guardian-specific document elsewhere?

* The general instructions about setting up branch Rulesets (push protection and CI requirement) are likely to apply to many _other_ Guardian repos (Scala apps, Typescript apps & libraries, etc), not just Scala library repos, so they probably don't belong in `gha-scala-library-release-workflow`, but should be well-documented elsewhere! The bypass exemption for the `gu-scala-library-release` bot definitely _does_ need to be documented, and that could live with that main Ruleset documentation, and be referenced with a pointer in the docs in `gha-scala-library-release-workflow`

@rtyley I have moved the instructions to a new page in the recommendations repo PR, and just added an extra point to the guardian instructions - hope this is ok?

Copy link
Member

@rtyley rtyley left a comment

Choose a reason for hiding this comment

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

Yeah, brilliant! Sorry it took me so long to get back to you, this is great 👍

@tjsilver tjsilver merged commit 9d21303 into main Apr 2, 2024
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