Skip to content

Allow triggering unstable buildbots with the !buildbot PR comment #419

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

Closed
itamaro opened this issue Oct 6, 2023 · 9 comments
Closed

Allow triggering unstable buildbots with the !buildbot PR comment #419

itamaro opened this issue Oct 6, 2023 · 9 comments

Comments

@itamaro
Copy link
Contributor

itamaro commented Oct 6, 2023

this came up several times recently around testing the NoGIL build on PRs - it's currently not possible to do that using the !buildbot nogil comment (example attempt) because the nogil builders are marked as unstable.
considering the NoGIL build is "experimental", keeping these builders as "unstable" is probably a good idea, but then we need some way to be able to request these builders on PRs.

after chatting with @ambv, I propose that we extend the !buildbot command to support unstable builders.
the mechanics can be discussed in this issue. here are the options I am considering:

  1. easy but changes existing behavior: include unstable buildbots by default
  2. also easy but kinda hacky: include unstable buildbots if they have a certain tag (e.g. "trigger-on-pr-command")
  3. introduce new spelling for explicitly allowing unstable builders (e.g. !buildbot_with_unstable ... or !buildbot[unstable] or !buildbot ... #with_unstable - bikeshedding welcome)
@vstinner
Copy link
Member

vstinner commented Oct 7, 2023

An alternative is to promote the buildbot to stable.

Do you want to propose a PR to make them STABLE?

@itamaro
Copy link
Contributor Author

itamaro commented Oct 7, 2023

I'm fine with changing these to STABLE, and happy to send a PR to make that change!
I was under the impression that marking a builder as STABLE has broader implications that are not appropriate for an experimental and optional build mode (not sure what my impression was based on, other than my own interpretation of the word "stable" :) )

@ambv
Copy link
Contributor

ambv commented Oct 7, 2023

You're correct, Itamar. Stable buildbots are special for release managers, so at least until the PEP is officially approved, I'd wait with marking nogil BBs as stable.

I believe that the limitation to only allow running stable buildbots with "!buildbot" is unnecessary.

@vstinner
Copy link
Member

vstinner commented Oct 7, 2023

Oh wait, I was confused about the definition of "stable" here.

For me, unstable means "is known to fail", and stable "is known to be reliable. Don't we have tiers to decide if a builder blocks a released or not? For example, Tier 3 failures do not block a release: https://peps.python.org/pep-0011/#tier-3

itamaro added a commit to itamaro/buildmaster-config that referenced this issue Oct 8, 2023
itamaro added a commit to itamaro/buildmaster-config that referenced this issue Oct 8, 2023
This intention of this PR is to make it possible to *manually trigger* also unstable buildbots on PRs using the `!buildbot` command (and labels).

The lists of "PR builders" are used in several places where I kept it to *only* stable PRs, because those other places didn't seem related to the manual triggering of buildbots on PRs.
itamaro added a commit to itamaro/buildmaster-config that referenced this issue Oct 8, 2023
This intention of this PR is to make it possible to *manually trigger* also unstable buildbots on PRs using the `!buildbot` command (and labels).

The lists of "PR builders" are used in several places where I kept it to *only* stable PRs, because those other places didn't seem related to the manual triggering of buildbots on PRs.
@itamaro
Copy link
Contributor Author

itamaro commented Oct 8, 2023

I read the Working with buildbots devguide page again, and it mentions stability in two places:

  1. In this section it mentions only stable buildbots will post a message to PRs that break them (makes sense, I wouldn't want to extend this to unstable buildbots)
  2. In the section conveniently named "Stability" it mentions stable buildbots are the ones taken into account when making releases

I agree with @vstinner that the semantics of stability vs tiers is confusing. If I combine the devguide with PEP-11, then only TIER_1 and TIER_2 builders can be STABLE, but in practice I see many TIER_3 and NO_TIER builders that are also considered STABLE.

Maybe further clarification is needed. In the meantime, I propose gh-420 to add the unstable builders only to those that can be manually triggered on PRs (@ambv's suggestion above).

vstinner added a commit to vstinner/buildmaster-config that referenced this issue Oct 11, 2023
vstinner added a commit to vstinner/buildmaster-config that referenced this issue Oct 11, 2023
@vstinner
Copy link
Member

I created PR #422 to fix the Release Status page: only list Tier-1 and Tier-2 builders.

Tier-3 and "No Tier" builders must be omitted there. For example, FreeBSD failures must not be listed there.

@itamaro
Copy link
Contributor Author

itamaro commented Oct 11, 2023

Thanks @vstinner! Should we do gh-420 too? or change the NoGIL builders to stable?

ambv pushed a commit that referenced this issue Oct 27, 2023
This intention of this PR is to make it possible to *manually trigger* also unstable buildbots on PRs using the `!buildbot` command (and labels).

The lists of "PR builders" are used in several places where I kept it to *only* stable PRs, because those other places didn't seem related to the manual triggering of buildbots on PRs.
itamaro added a commit to itamaro/buildmaster-config that referenced this issue Oct 30, 2023
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
vstinner pushed a commit that referenced this issue Oct 31, 2023
* GH-419: Pass unstable builders also into GitHubPrScheduler

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

* Try a different approach - make GitHubPrScheduler aware of which builders are stable

* Drop `NO_NOTIFICATION` check
itamaro added a commit to itamaro/buildmaster-config that referenced this issue Oct 31, 2023
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)
itamaro added a commit to itamaro/buildmaster-config that referenced this issue Nov 1, 2023
vstinner pushed a commit that referenced this issue Nov 1, 2023
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)
itamaro added a commit to itamaro/buildmaster-config that referenced this issue Nov 1, 2023
@itamaro
Copy link
Contributor Author

itamaro commented Nov 1, 2023

with GH-436 the triggering is finally working correctly. we're still not getting the status from unstable builders reported to GitHub Status thingie - this should be fixed with GH-437 (and get this issue resolved finally :) )

@itamaro
Copy link
Contributor Author

itamaro commented Nov 1, 2023

looks good on test PR python/cpython#111583 !

correct builders were triggered and their status is visible on GitHub checks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants