-
Notifications
You must be signed in to change notification settings - Fork 21
Feat: Extend to handle issues whose erratum has not been created #278
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
Conversation
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 looks along the lines I was thinking, some assorted comments.
supervisor/work_item_handler.py
Outdated
|
|
||
| def resolve_wait(self, why: str): | ||
| return WorkflowResult(status=why, reschedule_in=WAIT_DELAY) | ||
| def resolve_wait(self, why: str, *, reschedule_in: float | None = None): |
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's more transparent to do reschedule_in: float = WAIT_DELAY
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.
You're right! What I did here is totally unnecessary. 😅
supervisor/issue_handler.py
Outdated
| """ | ||
| async def run_before_errata_created(self) -> WorkflowResult: | ||
| issue = self.issue | ||
| labels_set = set(issue.labels) |
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.
The set of labels should be small enough that a few linear scans of if "jotnar_merged" in labels won't matter.
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.
Will change!
supervisor/issue_handler.py
Outdated
| logger.info("Running workflow for issue %s", issue.url) | ||
| else: | ||
| # TODO: We should not encounter this scenario | ||
| raise ValueError(f"Issue without target labels: {issue.labels}") |
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.
Things may have changed between when the collector runs and when an issue is processed, so the general structure of a handler is:
- Check if it still is matching the collector query
- If not, call resolve_remove_work_item()
- If yes, do stuff
(This actually implies to me that the collector should check for backported, rebased, or merged labels, since it's weird to me if the backported/rebased labels are removed but the merged label is left that we'd stop processing.)
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.
Got it! I'll change this to call resolve_remove_work_item(). 😁
supervisor/issue_handler.py
Outdated
| # add_issue_label( | ||
| # issue.key, | ||
| # "jotnar_merged", | ||
| # "MR has been merged", |
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 is not a particularly useful comment ... we don't need to have a comment, but I think in this case it would be useful, along the lines:
A [merge request| https://...]. resolving this issue has been merged; waiting for errata creation and final testing.
One thing that I'm thinking about is that it's quite possible and even normal for errata creation to happen before we get here; if we don't set the jotnar_merged label in that case, then we're going to make "dashboard" JIRA queries harder. So, I think we could have a function that if backported/rebased is set but not merged, checks for merged MR's and sets the merged flag, and call that from both branches.
It probably doesn't need to requeue - it can probably just set the label and let processing continue.
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 work on creating this function.
It probably doesn't need to requeue - it can probably just set the label and let processing continue.
I'll change to to call resolve_remove_work_item instead.
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 meant "just do whatever would happen next on the requeue immediately" - not remove the work item.
44b851b to
8feedde
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.
@owtaylor Marked a few places where I need your advice! Please let me know if there are any changes needed! Thanks! 😁
8feedde to
793f1b1
Compare
793f1b1 to
b94aba9
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.
I think we can tighten it up for organization a bit more.
b94aba9 to
878ddcf
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.
Getting down to the small stuff! It's looking geat.
This patch extends the supervisor to handle those issues without errata link.
We now fetch issues that contain labels in the following set:
- "jotnar_merged": Check if the MR has been merge for more than 24 hours,
if yes, flag attention. Otherwise, reschedule it for 1 hour in the
future.
- "jotnar_backported": Check if the MR has been merged, if yes, label it
"jotnar_merged". Otherwise, reschedule it for 3 hours in the future.
- "jotnar_rebased": Same as "jotnar_backported".
878ddcf to
b3d2452
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.
Looks great!
|
Thanks for the review! 😁 |
This patch extends the supervisor to handle those issues without errata link. We now fetch issues that contain labels in the following set: