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

RHEL-9: Migrate makebumpver script from Bugzilla to JIRA #5349

Merged

Conversation

jkonecny12
Copy link
Member

@jkonecny12 jkonecny12 commented Nov 28, 2023

Migrate our current makebumpver script from Bugzilla checking to JIRA.

We are changing the convention from:

Resolves: rhbz#<bug number>
Related: rhbz#<bug number>

to

Resolves: <RHEL issue>
Related: <RHEL issue>

This script is a bit outdated so let's take a brief change on this during the path:

  • Base migration to pep8 (still names of the variables and functions are not correct)
  • Backport fix to work with infra: ... together with ... (#infra) in the infra commits - improves backporting for us
  • Improve duplication in the code
  • Improve logging - less mess up output compared until you enable --debugging
  • Switch from sys.std*** to print

@jkonecny12 jkonecny12 requested a review from jstodola November 28, 2023 11:32
@jkonecny12 jkonecny12 changed the title RHEL-9: Migrate makebumpver script from Bugzilla to JIRA RHEL-9: Migrate makebumpver script from Bugzilla to JIRA Nov 28, 2023
@jkonecny12 jkonecny12 added port to Fedora port to RHEL8 blocked Don't merge this pull request! infrastructure Changes affecting mostly infrastructure labels Nov 28, 2023
@jkonecny12
Copy link
Member Author

Blocked by #5348 and #5351

@@ -1,3 +1,10 @@
# ======================================
# WARNING!
# THIS FILE IS GENERATED FROM A TEMPLATE
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

def validate_RHEL_issue_status(self, bug):
issue = self._query_RHEL_issue(bug)

valid_statuses = ("Planning", "In Progress", "Integration")
Copy link
Contributor

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.

Copy link
Member Author

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!

m = re.match(branch_pattern, self.git_branch)
if m:
return m.group(1)
rhel8_branch_pattern = r"^rhel-(\d+)(.*)"
Copy link
Contributor

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)

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

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.

summary = summary + f" ({author})"

for bodyline in body:
m = re.match(r"^(Resolves|Related):\ +jira#\w+-\d+.*$", bodyline)
Copy link
Contributor

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)?

Copy link
Member Author

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.

# 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")
Copy link
Contributor

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.

Copy link
Member Author

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.

return False

if self.git_branch.startswith('rhel'):
branch_pattern = r"^rhel(\d+)-(.*)"
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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.


rpm_log = []
bad_bump = False
bad = False
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

@jkonecny12
Copy link
Member Author

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 j2 file.

VladimirSlavik and others added 7 commits December 1, 2023 15:28
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.
@jkonecny12 jkonecny12 force-pushed the rhel-9-switch-makebumpver-to-jira branch from 0490a0d to 52bebed Compare December 1, 2023 14:31
@jkonecny12
Copy link
Member Author

Updated and prepared for review and hopefully merge :).

@jkonecny12 jkonecny12 removed the blocked Don't merge this pull request! label Dec 1, 2023
@jkonecny12
Copy link
Member Author

/kickstart-test --waive infra only

scripts/makebumpver Outdated Show resolved Hide resolved
scripts/makebumpver Outdated Show resolved Hide resolved
# 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")
Copy link
Contributor

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?

Copy link
Member Author

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.

@jkonecny12 jkonecny12 force-pushed the rhel-9-switch-makebumpver-to-jira branch 2 times, most recently from 7f937c1 to d2b3a3f Compare December 1, 2023 15:56
@jkonecny12
Copy link
Member Author

Fixed notes from the last review.

Copy link
Contributor

@jstodola jstodola left a comment

Choose a reason for hiding this comment

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

It looks OK.

@jkonecny12 jkonecny12 force-pushed the rhel-9-switch-makebumpver-to-jira branch from d2b3a3f to 2d6b007 Compare December 4, 2023 13:51
@jkonecny12
Copy link
Member Author

UPDATED: Moved the templating out of main script.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a 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.

scripts/makebumpver Outdated Show resolved Hide resolved
scripts/makebumpver Outdated Show resolved Hide resolved
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.
@jkonecny12 jkonecny12 force-pushed the rhel-9-switch-makebumpver-to-jira branch from 2d6b007 to bb68822 Compare December 5, 2023 11:58
@jkonecny12
Copy link
Member Author

Applied improvement from note from @VladimirSlavik and also found a bug in the code in error reporting - fixed now.

@jkonecny12
Copy link
Member Author

/kickstart-test --waive infra only

@jkonecny12 jkonecny12 merged commit 194e96c into rhinstaller:rhel-9 Dec 6, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Changes affecting mostly infrastructure rhel-9
Development

Successfully merging this pull request may close these issues.

4 participants