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

chore(ci): update ci #1784

Merged
merged 1 commit into from
Nov 20, 2023
Merged

chore(ci): update ci #1784

merged 1 commit into from
Nov 20, 2023

Conversation

mwangggg
Copy link
Member

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #1783

Copy link
Contributor

Hi @mwangggg! Add at least one of the required labels to this PR

Required labels are : chore,ci,cleanup,docs,feat,fix,perf,refactor,style,test

@mergify mergify bot added the safe-to-test label Nov 20, 2023
Copy link
Contributor

Hi @mwangggg! Add at least one of the required labels to this PR

Required labels are : chore,ci,cleanup,docs,feat,fix,perf,refactor,style,test

@mwangggg
Copy link
Member Author

one thing- I added the permissions: pull-requests: write to each job separately, but wondering if it would be better to just include the permission for the whole workflow?

@mwangggg mwangggg added ci chore Refactor, rename, cleanup, etc. labels Nov 20, 2023
@mwangggg
Copy link
Member Author

mwangggg commented Nov 20, 2023

Also, all the other repos use status checks instead of comments. For consistency, should I update Cryostat to use status checks instead of, or on top of, the comments that are currently being printed? Or, if we think comments are better, I can update the other way around. @aali309 @andrewazores

@andrewazores
Copy link
Member

but wondering if it would be better to just include the permission for the whole workflow?

I would rather keep it granular and applied only to each specific job/step that actually needs it. Otherwise, we may as well just leave the default setting to grant it all the time if we're going to broadly request it at the workflow level everywhere.

@andrewazores
Copy link
Member

Also, all the other repos use status checks instead of comments. For consistency, should I update Cryostat to use status checks instead of, or on top of, the comments that are currently being printed? Or, if we think comments are better, I can update the other way around.

Status checks are good because they allow PR merge to be gated, ie the "Merging is blocked" status can be made dependent on the outcome of these workflows, which is super nice.

On the other hand, comments are nice because it lets us add various pieces of detailed information. It might be possible to add that to the statuses as well but then you have to click through somewhere else to see the details, and the history of those statuses probably won't be as clear over time as the comment history is.

So IMO both have their place and I'd like to have both where it makes sense. If a repo has a simple CI setup with no /build_test etc. then the statuses alone are fine, but once it gets a bit more advanced, comments are good to add.

@andrewazores andrewazores merged commit 6b43da7 into cryostatio:main Nov 20, 2023
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. ci safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Meta] CI workflows must request write permissions
2 participants