-
Notifications
You must be signed in to change notification settings - Fork 356
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
RHEL-9: Migrate makebumpver
script from Bugzilla to JIRA
#5349
RHEL-9: Migrate makebumpver
script from Bugzilla to JIRA
#5349
Conversation
makebumpver
script from Bugzilla to JIRA
scripts/makebumpver
Outdated
@@ -1,3 +1,10 @@ | |||
# ====================================== | |||
# WARNING! | |||
# THIS FILE IS GENERATED FROM A TEMPLATE |
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 wonder why is this file needed to be in git, since it can be generated anytime from the template.
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.
It is one to be used. Our templating system works like:
- The template file
.j2
is the only file you should ever modify - The output file (this one) is what you should use
- Both are committed because otherwise you would have to generate the file every time you need to use it. Which makes especially automation harder.
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.
Automation could generate makebumpver
automatically from the .j2
template before use. My worry is that any modification has to be done in both files and committed to git to keep the files in sync. With just the template you do not have such a problem. But if keeping the files in sync is no big deal and modifications are rare, why not.
scripts/makebumpver.j2
Outdated
def validate_RHEL_issue_status(self, bug): | ||
issue = self._query_RHEL_issue(bug) | ||
|
||
valid_statuses = ("Planning", "In Progress", "Integration") |
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.
IMO, you should not make modifications to bugs in the "Integration" state - such a bug is in hands of QE and no changes should be made there any more. The correct way is to move the bug back to "In Progress" and then make the necessary changes.
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 wasn't sure about this one. Thanks for the reply!
scripts/makebumpver.j2
Outdated
m = re.match(branch_pattern, self.git_branch) | ||
if m: | ||
return m.group(1) | ||
rhel8_branch_pattern = r"^rhel-(\d+)(.*)" |
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 find rhel8_branch_pattern
confusing, branch names for both RHEL-8 and RHEL-9 look the same (rhel-8
, rhel-9
)
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.
Again, maybe a separate PR, but I would be also fine with this PR.
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.
Seems like a leftover from the original parts of the script :D
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'll add a new commit fixing this on the pile.
scripts/makebumpver.j2
Outdated
summary = summary + f" ({author})" | ||
|
||
for bodyline in body: | ||
m = re.match(r"^(Resolves|Related):\ +jira#\w+-\d+.*$", bodyline) |
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.
Why not allow also "Resolves:jira#RHEL-11111" (without the space)?
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.
We never had that as the convention and I don't think we want to start with that now.
scripts/makebumpver.j2
Outdated
# store the bug to output list if checking is disabled and continue | ||
if self.skip: | ||
issues.add(f"{action}: jira#{bug}") | ||
print(f"*** Bug {bug} Related commit {commit} is allowed\n") |
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.
It's not checked if the commit is allowed, so the log message should be updated accordingly.
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 message is similar to what was the original message, however, I agree it could be improved now. Will do.
scripts/makebumpver.j2
Outdated
return False | ||
|
||
if self.git_branch.startswith('rhel'): | ||
branch_pattern = r"^rhel(\d+)-(.*)" |
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.
Is this branch pattern still used?
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 think it is not. Maybe I'd remove it in a follow-up separate PR, but I'd be fine with a commit in this set as well.
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 really important. The script will not be moved to RHEL-10 it will go there from master. Let's fix this on master separately.
scripts/makebumpver.j2
Outdated
|
||
rpm_log = [] | ||
bad_bump = False | ||
bad = False |
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.
Maybe we can get rid of one of the bad
and bad_bump
variables now.
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 point.
I see what is happening. By creating a j2 template file all the lines changed not just the lines I touched, so I'm getting review also for parts which doesn't changed by me and they are just copied over. I guess that is fine. @jstodola the easiest way to do the review (based on my experience) is to do that by commit and if it is move the code to j2 file. You can just take a look on the generated file which will show you the changes and completely skip reading of the |
(cherry picked from commit 1769a7d)
(cherry picked from commit 5102cae)
Let's make this consistent through the file.
Let's make the method a bit more sane by moving the bug checking code out of giant if clause.
Let's make it a function instead of copied code on plenty of places.
Without this change we will get all the debug outputs from the bugzilla JIRA calls which makes the output hard to read. Let's enable these only in debugging mode.
0490a0d
to
52bebed
Compare
Updated and prepared for review and hopefully merge :). |
/kickstart-test --waive infra only |
scripts/makebumpver.j2
Outdated
# store the bug to output list if checking is disabled and continue | ||
if self.skip: | ||
issues.add(f"{action}: jira#{bug}") | ||
print(f"*** Bug {bug} Related commit {commit} is skipped\n") |
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.
Is the commit really skipped? Just the checks are skipped, aren't they?
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 see, yeah, it might be more clear with skipping checks
probably.
7f937c1
to
d2b3a3f
Compare
Fixed notes from the last review. |
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.
It looks OK.
d2b3a3f
to
2d6b007
Compare
UPDATED: Moved the templating out of main script. |
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. Thank you for not changing the whole file into a template :)
There are some nitpicks, but feel free to ignore these.
Validate JIRA issues on commits with Red Hat JIRA and not to bugzilla anymore. From RHEL-8.10 and RHEL-9.4 Bugzilla is not used by Red Hat any JIRA is the successor. Let's adapt this script to that. It's simplified version of RHBZ solution. What is different: * JIRA don't have ACKs but instead states * JIRA don't support Related / Resolves - still in the code because it could help us but JIRA is just taking the issue and ignores the rest. * Don't support BZ in summary (we are not using that anymore) * Print confirmation that the given commit is correct
Remove Bugzilla code which was migrated to JIRA and remove arguments which can't be reasonably used with JIRA. Remove: --map Mapping between Fedora and RHEL bugs is functionality which is not used anymore and it can't be even reasonably used from Bugzilla to JIRA. --skip-acks and --skip-all was renamed to --skip ACKs are not used on JIRA anymore so having two different skip parameters doesn't help. unused variable bad_bump
Let's make the script working between branching by getting the version validation from the templates.
We don't use Bugzilla anymore for RHEL bugs. Bugzilla was migrated to JIRA.
Fix the variable naming. We are not on RHEL-8 anymore :).
If user is missing python3-jira library let's tell them where to get it.
2d6b007
to
bb68822
Compare
Applied improvement from note from @VladimirSlavik and also found a bug in the code in error reporting - fixed now. |
/kickstart-test --waive infra only |
Migrate our current
makebumpver
script from Bugzilla checking to JIRA.We are changing the convention from:
to
This script is a bit outdated so let's take a brief change on this during the path:
infra: ...
together with... (#infra)
in the infra commits - improves backporting for ussys.std***
toprint