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.

Sign up for GitHub

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

Jump to bottom

[Merge-Queue] Skip CloseBug if no bug defined #231

Closed
wants to merge 1 commit into from
Closed

[Merge-Queue] Skip CloseBug if no bug defined #231

wants to merge 1 commit into from

Conversation

JonWBedard
Copy link
Member

@JonWBedard JonWBedard commented Mar 31, 2022 •
edited by webkit-early-warning-system

6ddf728

[Merge-Queue] Skip CloseBug if no bug defined
https://bugs.webkit.org/show_bug.cgi?id=238612
<rdar://problem/91110116 >

Reviewed by Aakash Jain.

* Tools/CISupport/ews-build/steps.py:
(CloseBug.start): Ignore unavailable bug_id case.
(CloseBug.getResultSummary): Use default summary for skip case.
(CloseBug.doStepIf): Do step if bug_id is available.
(CloseBug.hideStepIf): Hide step if skipped.

Canonical link: https://commits.webkit.org/249068@main

From: Jonathan Bedard <jbedard@apple.com >


Canonical link: https://commits.webkit.org/249068@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292161 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@JonWBedard JonWBedard self-assigned this Mar 31, 2022
@JonWBedard JonWBedard requested a review from aj062 March 31, 2022 17:24
@JonWBedard JonWBedard force-pushed the eng/Merge-Queue-Skip-CloseBug-if-no-bug-defined branch from 110d6e2 to facb27c Compare March 31, 2022 17:27
aj062
aj062 approved these changes Mar 31, 2022
View reviewed changes
Copy link
Member

@aj062 aj062 left a comment

Choose a reason for hiding this comment

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

r+ with comments.

Tools/CISupport/ews-build/steps.py Outdated
self.descriptionDone = 'No bug id found'
self.finished(FAILURE)
return None
def __init__(self, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

seems like this init can be removed, can initialize bug_id as class variable or can handle it appropriately in getResultsummary as well. But this is fine as well.

Tools/CISupport/ews-build/steps.py
rc = self.close_bug(self.bug_id)
self.finished(rc)
return None

def getResultSummary(self):
if self.results == SKIPPED:
Copy link
Member

Choose a reason for hiding this comment

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

Actually it's better not to have this custom result summary for SKIPPED case. First of all it wouldn't be displayed in the buildbot UI, since we set hideStepIf as well. Secondly, this custom string will show up in status-bubble pop-over message, while the default one (ending in (skipped)) is hidden (See: https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-app/ews/views/statusbubble.py#L70)

@aj062 aj062 mentioned this pull request Mar 31, 2022
Closed
aj062
aj062 reviewed Mar 31, 2022
View reviewed changes
Tools/CISupport/ews-build/steps.py
rc = self.close_bug(self.bug_id)
self.finished(rc)
return None

def getResultSummary(self):
if self.results == SKIPPED:
return {'step': 'No bug to close'}
if self.results == SUCCESS:
return {'step': 'Closed bug {}'.format(self.bug_id)}
return {'step': 'Failed to close bug {}'.format(self.bug_id)}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I missed noticing this default case handling (in this and previous few PRs). It doesn't look good. You should explicitly handle the cases which you want, and let buildbot default getResultSummary() handle the rest.

e.g.: https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-build/steps.py#L882 or https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-build/steps.py#L2834

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation here, will be updating these later today.

@JonWBedard JonWBedard force-pushed the eng/Merge-Queue-Skip-CloseBug-if-no-bug-defined branch 2 times, most recently from a8bea09 to 0dc3660 Compare March 31, 2022 18:06
@JonWBedard JonWBedard added Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases WebKit Nightly Build merge-queue Applied to send a pull request to merge-queue labels Mar 31, 2022
@webkit-early-warning-system
Copy link
Collaborator

commit-queue failed to commit attachment to WebKit repository. To retry, please set cq+ flag again.

@webkit-early-warning-system webkit-early-warning-system added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Mar 31, 2022
@JonWBedard JonWBedard added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Mar 31, 2022
@JonWBedard
[Merge-Queue] Skip CloseBug if no bug defined
8b5acb6 
https://bugs.webkit.org/show_bug.cgi?id=238612
<rdar://problem/91110116>

Reviewed by Aakash Jain.

* Tools/CISupport/ews-build/steps.py:
(CloseBug.start): Ignore unavailable bug_id case.
(CloseBug.getResultSummary): Use default summary for skip case.
(CloseBug.doStepIf): Do step if bug_id is available.
(CloseBug.hideStepIf): Hide step if skipped.
@webkit-early-warning-system
Copy link
Collaborator

Committed r292161 (249068@main): https://commits.webkit.org/249068@main

All reviewed patches have been landed. Closing bug and clearing flags on attachment .

@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Merge-Queue-Skip-CloseBug-if-no-bug-defined branch from 0dc3660 to 8b5acb6 Compare March 31, 2022 18:24
@JonWBedard JonWBedard removed the merge-queue Applied to send a pull request to merge-queue label Mar 31, 2022
@JonWBedard JonWBedard closed this Mar 31, 2022
@JonWBedard JonWBedard deleted the eng/Merge-Queue-Skip-CloseBug-if-no-bug-defined branch March 31, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aj062 aj062 aj062 approved these changes

Assignees

@JonWBedard JonWBedard

Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
Milestone
No milestone
3 participants
@JonWBedard @webkit-early-warning-system @aj062