test-infra

test-infra copied to clipboard

This commit adds support for closing issues as Not Planned by utilising the new state_reason attribute of the GH API. The default value of this attribute, if not set, is completed which is the current behaviour.

Changes to the github client:

• The IssueClient interface is modified to include a method called CloseIssueAsNotPlanned which sets the state_reason attribute to not_planned
• The Issue type is also modified to now have the StateReason field.
• Subsequent changes to the fake client are made to set the StateReason field to completed or not_planned depending on the method invoked.

Changes to lifecycle plugin:

• The /close command is extended to be used as /close not-planned if an issue is to be closed as Not Planned.

Ref #26380

---

/assign @nikhita @cblecker /cc @mrbobbytables @jberkus

Also, tried this out in a playground gh org, test issue link: https://github.com/uwubernetes/label-sync-test/issues/1

Command:

curl -X PATCH -H "Authorization: token <TOKEN>" \                                                                     
  https://api.github.com/repos/uwubernetes/label-sync-test/issues/1 \
  -d '{"state":"closed","state_reason":"not_planned"}'

For some reason the state_reason attribute hasn't made its way into the API docs yet :/

/sig contributor-experience

+1 from me on the general idea, but I'm going to leave it to our GH admins to decide on the code.

Couple of comments, but LGTM otherwise. Thanks for working on this, @MadhavJivrajani!

Done, thanks for the review @nikhita!

@palnabarun: Closed this PR.

In response to this:

This regex will match the following which feels a little odd to allow.

/close




not-planned

If users are not allowed to have new lines between /close and not-planned, the regex needs to be something like

regexp.MustCompile(`(?mi)^/close not-planned\s*$`) // allows only one space
(or)
regexp.MustCompile(`(?mi)^/close[ ]+not-planned\s*$`) // allowed one or more spaces (which feels a little weird too)

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/test-infra repository.

Awesome! This proves the regex needs to be improved :p

/reopen

@MadhavJivrajani: Reopened this PR.

In response to this:

Awesome! This proves the regex needs to be improved :p

/reopen

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/test-infra repository.

@palnabarun I've addressed your comments, thanks!

Hi, can we merge this? I want to use this feature

/approve

Nice work @MadhavJivrajani - thank you for the contribution!

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MadhavJivrajani, palnabarun, stevekuznetsov

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:

• ~~prow/OWNERS~~ [stevekuznetsov]

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

@MadhavJivrajani: Reopened this PR.

In response to this:

Awesome! This proves the regex needs to be improved :p

/reopen

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/test-infra repository.