GH-419: Pass unstable builders also into GitHubPrScheduler #434
Conversation
This is needed to support triggering unstable builders using `!buildbot` PR comment Add the stability to the builder name so we can limit label-based triggering only to stable builders
master/master.cfg
Outdated
| stable_pull_request_builders.append(buildername) | ||
|
|
||
| else: | ||
| unstable_pull_request_builders.append(buildername) | ||
|
|
||
| if any(pattern in buildername for pattern in NO_NOTIFICATION): | ||
| continue |
There was a problem hiding this comment.
I think if you axe this check, we'd land about where we want to be.
There was a problem hiding this comment.
I tried swapping the order of this check with adding the builder name to the list - that passes the config check!
it would work to axe it as well, but that would change the behavior for builders that match the patterns in NO_NOTIFICATION, and I don't know if that's a desirable change (I'm assuming there's a reason for that).
There was a problem hiding this comment.
with either change (swap order or axe it), I think either approach would work - do you have a preference which one to take?
- add "stability" to the builder name and use it in the regex; or
- the current state of this PR - use an explicit set of "stable builders" when not doing comment-based triggering
There was a problem hiding this comment.
I'd say 2.
I think we can do away with NO_NOTIFICATION. The Alpine and UBSan builders are unstable, so should be left out of label builds anyway, and we no longer have a Cygwin builder. I could see value in being able to manually trigger the Alpine or UBSan builders via a comment.
There was a problem hiding this comment.
Ok, scratch that: we can't quite do away with NO_NOTIFICATION, but we can drop the check on it here.
There was a problem hiding this comment.
ok, I dropped the check, I think this should be good to go!
694b7e9 to
25f7fe1
Compare
| builder_name | ||
| for builder_name in builder_names | ||
| if builder_name in self.stable_builder_names | ||
| ] |
There was a problem hiding this comment.
Would it be possible to create the 2 build name lists only once in the constructor, rather than having to do it at each GitHub event?
There was a problem hiding this comment.
not in the general case - you can see in line 26 the initial value of builder_names can come from the kwargs of the addBuildsetForChanges call, so we can't assume anything in the constructor.
I am not an expert on buildbot internals, but I don't actually see where addBuildsetForChanges can be called with a builderNames kwarg. if we only ever use self.builderNames in practice, we can definitely pre-construct the list in the constructor.
I can make this change now, or perhaps add logging so we can see whether it is ever called with a builderNames kwarg.
There was a problem hiding this comment.
Oh ok, I was wrong, ignore my comment.
|
I merged your PR, thanks @itamaro. |
pythonGH-434 attempted to support triggering unstable builder with comment-based triggering It had two flaws that are fixed in this follow-up PR: 1. Needed to unpack the `event` property 2. If we are left with no builders after filtering, need to avoid the super() call (otherwise we end up triggering ALL builders O_O)
GH-434 attempted to support triggering unstable builder with comment-based triggering It had two flaws that are fixed in this follow-up PR: 1. Needed to unpack the `event` property 2. If we are left with no builders after filtering, need to avoid the super() call (otherwise we end up triggering ALL builders O_O)
This is needed to support triggering unstable builders using
!buildbotPR commentPass
GitHubPrSchedulera set of "known stable builders" to it can avoid triggering unstable builders unless it's a comment-based trigger