-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
CI: On vX.Y-rhel branches, ensure that some downstream Jira issue is linked #23622
CI: On vX.Y-rhel branches, ensure that some downstream Jira issue is linked #23622
Conversation
@baude @mheon @TomSweeneyRedHat @edsantiago follow up from the email discussion. The exact syntax, as well as the label name for the override is just an initial proposal for me, and is up for discussion. |
8f59aaf
to
c0d5bfd
Compare
Btw, the DEST_BRANCH in .cirrus.yml on the "v4.9-rhel" branch is wrong (it is "v4.9"), and we have to update it if we want this to work there. |
I'm thinking we're going to mandate all PRs on upstream that fix an issue reference that issue as "Fixes " where issue could be a Github issue ID, a URL, or a Jira reference. So I think matching "Fixes Jira -\n" would be preferable. But the overall approach LGTM. |
@cevich Is there any way we can enforce Cirrus DEST_BRANCH being set correctly, because I think we're especially bad about remembering to do this for -rhel branches? |
Maybe we can have a github action (not in cirrus) that validates the cirrus DEST_BRANCH value? I think you can get that from $GITHUB_BASE_REF |
That's not a bad suggestion actually since github actions on this repo can send e-mail to our monitoring list. OTOH, the base-branch name is exposed in a Cirrus-CI env. var. (can't recall the exact name) to PR-level tasks but nowhere else (i.e. not in tag or branch-level jobs). So maybe a simple check could be done if |
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.
Not against this per se, just marking that I don't think we should merge this until we have a general accept format defined and documented before we "enforce" anything
@Luap99 I'm going to proposal the generic |
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 needs to be documented somwehere. Perhaps https://github.com/containers/podman/blob/main/CONTRIBUTING.md
Any chance I can join that? |
@alexlarsson Sure, I can invite you. |
c0d5bfd
to
b91bfaa
Compare
Hmm, github actually also allows a colon, like "Fixes: ", and that is what CONTRIBUTING.md says. I'll make the PR handle that as well. |
b91bfaa
to
9cb44e5
Compare
@mheon Updated the PR |
Someone who is better at regexes than I should review, but LGTM |
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.
Good start, but a few suggestions inline
82f252c
to
4473d75
Compare
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.
LGTM. One gripe, not worth a repush. Thank you!
contrib/cirrus/pr-should-link-jira.t
Outdated
fi | ||
fi | ||
|
||
export CIRRUS_REPO_CLONE_TOKEN |
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 doesn't belong here (it should be on line 11) but it's not worth yet another repush.
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 added it there so i could test it with:
CIRRUS_REPO_CLONE_TOKEN=... ./pr-should-link-jira.t
Otherwise it would not get propagated to the test.
That said, one might very well not want to support that...
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.
If you do want to keep this here, I would be helpful to add a comment suggesting it's needed for pr-should-link-jira.t to function.
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 moved it.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexlarsson, edsantiago The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The mac build always seems to time out on the linter. I assume that is not my fault? |
I think there was a fix for that recently and rebasing on main should fix it. I might be wrong though. |
6493cca
to
ba46e56
Compare
I tried rebasing, lets see. |
Cockpit tests failed for commit ba46e5651476150655ba3f250b230110a5542f6b. @martinpitt, @jelly, @mvollmer please check. |
…linked In the RHEL specific branches we want to ensure that all MRs link to at least one downstream Jira ticket. To do this we add a new test in validate-source similar to the existing pr-should-include-tests. This test only runs on actual pull requests. The syntax for linking to a Jira is "Fixes " or "Fixes: ", followed by one jira links, like so: ``` Fixes https://issues.redhat.com/browse/RHEL-50506 Fixes: https://issues.redhat.com/browse/RHEL-50506 ``` Note: This is the same syntax as for a regular github issue reference. Signed-off-by: Alexander Larsson <alexl@redhat.com>
ba46e56
to
17193af
Compare
Still LGTM |
Ok, all requested changes are done, and now we passed CI. Can I get this merged? |
(Note: Once this is merged we need to backport to active rhel branches. |
/lgtm from me |
That might have confused the bot |
…linked This is a backport of #17193af962453a3cfde2d9f25c0fea29e3c91cf3 (from containers#23622) In the RHEL specific branches we want to ensure that all MRs link to at least one downstream Jira ticket. To do this we add a new test in validate-source similar to the existing pr-should-include-tests. This test only runs on actual pull requests. The syntax for linking to a Jira is "Fixes " or "Fixes: ", followed by one jira links, like so: ``` Fixes https://issues.redhat.com/browse/RHEL-50506 Fixes: https://issues.redhat.com/browse/RHEL-50506 ``` Note: This is the same syntax as for a regular github issue reference. Signed-off-by: Alexander Larsson <alexl@redhat.com>
Thanks for taking this on and following it through @alexlarsson |
…linked This is a backport of #17193af962453a3cfde2d9f25c0fea29e3c91cf3 (from containers#23622) In the RHEL specific branches we want to ensure that all MRs link to at least one downstream Jira ticket. To do this we add a new test in validate-source similar to the existing pr-should-include-tests. This test only runs on actual pull requests. The syntax for linking to a Jira is "Fixes " or "Fixes: ", followed by one jira links, like so: ``` Fixes https://issues.redhat.com/browse/RHEL-50506 Fixes: https://issues.redhat.com/browse/RHEL-50506 ``` Note: This is the same syntax as for a regular github issue reference. Signed-off-by: Alexander Larsson <alexl@redhat.com>
In the RHEL specific branches we want to ensure that all MRs link to at least one downstream Jira ticket. To do this we add a new test in validate-source similar to the existing pr-should-include-tests. This test only runs on actual pull requests.
The syntax for linking to a Jira is "Fixes " followed by one jira links, like so:
Note: This is the same syntax as for a regular github issue reference.