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

approvers: add commenter that marks stale PRs #3655

Merged
merged 2 commits into from
Sep 23, 2024
Merged

approvers: add commenter that marks stale PRs #3655

merged 2 commits into from
Sep 23, 2024
+45 −0

Conversation

dhiller
Copy link
Contributor

@dhiller dhiller commented Sep 17, 2024 •
edited
Loading

What this PR does / why we need it:

Adds a periodic job that will apply the label needs-approver-review to PRs that haven't received a review by an approver within a week.

Since that label doesn't exist yet, it is added as well - furthermore the documentation page is updated.

Finally a configuration to allow the bot user to apply the label is added in the label plugin config section.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

@dhiller
approvers: add commenter that marks stale PRs
8dfccca 
Adds a periodic job using the commenter to mark PRs that haven't
received a review by an approver within a week with the
needs/approver-review label.

Since that label doesn't exist yet, it is added as well - furthermore
the documentation page is updated.

Signed-off-by: Daniel Hiller <[email protected]>
@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Sep 17, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Sep 17, 2024

/cc @alicefr @iholder101

@alicefr
Copy link
Member

alicefr commented Sep 17, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Sep 17, 2024

/hold

This will not work, since the label plugin does allow to manipulate only certain kinds of default labels. I need to add a configuration that allows to add the label, but currently I am not so sure whether the chosen label is the best idea.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2024
@dhiller
approvers: rename label from needs/approver-review
d662df8 
Since the initial approach (using the `/needs ...`) is not supported
by the label plugin, we use the `/label` command. This requires a
configuration in the label plugin section to allow the bot user to set
the label on PRs, which we add here.

Finally we update the labels.md page with the latest changes.

Signed-off-by: Daniel Hiller <[email protected]>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Sep 17, 2024

/unhold

Updates: changed the label to needs-approver-review, added a label configuration to allow the bot user to apply the label with a comment.

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2024
@alicefr
Copy link
Member

alicefr commented Sep 18, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2024
iholder101
iholder101 reviewed Sep 18, 2024
View reviewed changes
github/ci/prow-deploy/files/jobs/kubevirt/project-infra/project-infra-periodics.yaml
is:pr
label:lgtm
-label:approved
-label:do-not-merge/work-in-progress
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to also add -label:do-not-merge/hold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that there might be cases where we don't want to exclude PRs that have a hold - i.e. this PR would still require reviewer attention, while it has an LGTM: kubevirt/community#297

(might be a special case though)

source query

@iholder101
Copy link
Contributor

/lgtm
Thank you @dhiller

brianmcarey
brianmcarey reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

/approve

thanks @dhiller

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianmcarey

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Sep 23, 2024

/override pull-project-infra-prow-deploy-test

due to check instability: #3667

@kubevirt-bot
Copy link
Contributor

@dhiller: Overrode contexts on behalf of dhiller: pull-project-infra-prow-deploy-test

In response to this:

/override pull-project-infra-prow-deploy-test

due to check instability: #3667

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kubevirt-bot kubevirt-bot merged commit 0d3bb5c into kubevirt:main Sep 23, 2024
7 checks passed
@kubevirt-bot
Copy link
Contributor

@dhiller: Updated the following 3 configmaps:

  • job-config configmap in namespace kubevirt-prow at cluster default using the following files:
    • key project-infra-periodics.yaml using file github/ci/prow-deploy/files/jobs/kubevirt/project-infra/project-infra-periodics.yaml
  • label-config configmap in namespace kubevirt-prow at cluster default using the following files:
    • key labels.yaml using file github/ci/prow-deploy/kustom/base/configs/current/labels/labels.yaml
  • plugins configmap in namespace kubevirt-prow at cluster default using the following files:
    • key plugins.yaml using file github/ci/prow-deploy/kustom/base/configs/current/plugins/plugins.yaml

In response to this:

What this PR does / why we need it:

Adds a periodic job that will apply the label needs-approver-review to PRs that haven't received a review by an approver within a week.

Since that label doesn't exist yet, it is added as well - furthermore the documentation page is updated.

Finally a configuration to allow the bot user to apply the label is added in the label plugin config section.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iholder101 iholder101 iholder101 left review comments

@brianmcarey brianmcarey brianmcarey left review comments

@enp0s3 enp0s3 Awaiting requested review from enp0s3

@alicefr alicefr Awaiting requested review from alicefr

Assignees

@alicefr alicefr

@iholder101 iholder101

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhiller @alicefr @iholder101 @kubevirt-bot @brianmcarey