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

CI require a 'lgtm' or 'ok-to-test' labels to run #11251

Merged
merged 1 commit into from
May 31, 2024

Conversation

ant31
Copy link
Contributor

@ant31 ant31 commented May 30, 2024

  • Implement script changes to require a 'lgtm' or 'ok-to-test' label for running CI
  • Update the 'moderator' stage rules to not run the job by default unless specific conditions are met

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
To stop running 'expensive' CI on all PR without a maintainer review.

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 30, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2024
@ant31 ant31 force-pushed the branch-pipeline branch 4 times, most recently from 71b39f6 to 95b5207 Compare May 30, 2024 12:41
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 30, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ant31

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 30, 2024
@VannTen
Copy link
Contributor

VannTen commented May 30, 2024

Is that going to work for the following :

  • new PR -> initial needs-ok-to-test
  • some time passes
  • org member adds the label (via comment and the prow automation) ok-to-test or lgtm -> will the PR trigger at that point ?

@ant31 ant31 force-pushed the branch-pipeline branch 3 times, most recently from 7506379 to 94fa505 Compare May 30, 2024 13:01
@ant31
Copy link
Contributor Author

ant31 commented May 30, 2024

org member adds the label (via comment and the prow automation) ok-to-test or lgtm -> will the PR trigger at that point

yes
I was thinking of having a /retest too to 're-trigger' i.
Labels events are received too and could be use also to trigger it automatically.

Created this PR first to check if the labels are published and can be used for rules.

@ant31 ant31 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2024
@ant31
Copy link
Contributor Author

ant31 commented May 30, 2024

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 30, 2024
@ant31 ant31 added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. labels May 30, 2024
@ant31
Copy link
Contributor Author

ant31 commented May 30, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 30, 2024
- Require a 'lgtm' or 'ok-to-test' label for running CI after the
  moderator stage

Signed-off-by: ant31 <2t.antoine@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 30, 2024
@ant31
Copy link
Contributor Author

ant31 commented May 30, 2024

/assign @VannTen

The pipeline is retried automatically when the labels "lgtm" or "ok-to-test" are applied
otherwise it will be blocked at the moderator stage.

@ant31
Copy link
Contributor Author

ant31 commented May 30, 2024

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2024
@VannTen
Copy link
Contributor

VannTen commented May 30, 2024 via email

@VannTen
Copy link
Contributor

VannTen commented May 30, 2024 via email

@ant31
Copy link
Contributor Author

ant31 commented May 31, 2024

Maybe it should be has: lgtm or not(has:needs-ok-to-test) otherwise it would block CI for org members right ?

Org member can apply themselve ok-to-test, why would it block CI ?

do-not-merge/work-in-progress

I think it's fine, the ok-to-test is explicit, you either have it applied or not.
I don't want to have to complicate rules

@ant31
Copy link
Contributor Author

ant31 commented May 31, 2024

I suggest we give it a try and chant it quickly if something is not working or annoying.

@VannTen
Copy link
Contributor

VannTen commented May 31, 2024

Fine by me, that was mainly for consistency with the rest of kubernetes-sigs where the ok-to-test is implicit for org-members.
One last question:
What happens if a PR modify the .gitlab-ci.yml ? Does that allow to bypass the check ?

@ant31
Copy link
Contributor Author

ant31 commented May 31, 2024

What happens if a PR modify the .gitlab-ci.yml

@VannTen, yes, it has always been the case.

To fix that, failfast-ci would have to 'fail' the sync before and not trigger the pipeline at all, which is doable.
Ideally that would happen only if any of the ci files is modified.

@VannTen
Copy link
Contributor

VannTen commented May 31, 2024 via email

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2024
@ant31
Copy link
Contributor Author

ant31 commented May 31, 2024

your /lgtm has triggered the pipeline -_- .

I'll try to fix that in a follow-up to not run twice( one for ok-to-test, one for lgtm)

@k8s-ci-robot k8s-ci-robot merged commit ef6d24a into kubernetes-sigs:master May 31, 2024
78 checks passed
@ant31 ant31 deleted the branch-pipeline branch May 31, 2024 12:05
ehsan310 pushed a commit to ehsan310/kubespray that referenced this pull request May 31, 2024
…1251)

- Require a 'lgtm' or 'ok-to-test' label for running CI after the
  moderator stage

Signed-off-by: ant31 <2t.antoine@gmail.com>
Rickkwa pushed a commit to Rickkwa/kubespray that referenced this pull request Jun 26, 2024
…1251)

- Require a 'lgtm' or 'ok-to-test' label for running CI after the
  moderator stage

Signed-off-by: ant31 <2t.antoine@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants