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

Add close-issue-reason option #764

Merged
merged 8 commits into from
Jun 23, 2022
Merged

Add close-issue-reason option #764

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e7ea989
Add close-as-not-planned option
jtbandes Jun 23, 2022
70ffa7f
update readme
jtbandes Jun 23, 2022
88af49e
add to Option enum
jtbandes Jun 23, 2022
830e366
improve wording
jtbandes Jun 23, 2022
e1eb218
npm run pack
jtbandes Jun 23, 2022
4e102e6
updates from review
jtbandes Jun 23, 2022
f83dd8d
fix tests and improve error message
jtbandes Jun 23, 2022
70f12f5
fix readme order
jtbandes Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ Every argument is optional.
| [ignore-updates](#ignore-updates) | Any update (update/comment) can reset the stale idle time on the issues/PRs | `false` |
| [ignore-issue-updates](#ignore-issue-updates) | Override [ignore-updates](#ignore-updates) for issues only | |
| [ignore-pr-updates](#ignore-pr-updates) | Override [ignore-updates](#ignore-updates) for PRs only | |
| [close-as-not-planned](#close-as-not-planned) | Use the "not planned" close reason for issues | |
Copy link

Choose a reason for hiding this comment

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

IMO because this only affects issues (PR closed state is either unmerged or merged), and because of the existence of options like close-issue-message, close-issue-label, etc. This option should be close-issue-<something>.

Also, right now it's a boolean (either not planned or completed), but that can change if GitHub ever introduces a 3rd closed state. That's why I suggested in #744 (comment) to have a close-issue-reason option that takes a string.

But don't make any changes just for me 🙂
Let's wait and see what the maintainers think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I think I like the close-issue-reason improvement, which makes us a little more resilient/future proof to any new states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you want to validate against a known set of values, or just pass it through to the API verbatim?

Copy link
Collaborator

@luketomlinson luketomlinson Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think validating against known values seems like a good idea. I haven't explored what happens if you were to pass state_reason: foo, so it's possible the API throws out bad values, but it's very possible it might also throw a 400.

So I think throwing out any values that aren't in the list ["closed", "not_planned"] makes sense.

Copy link
Contributor Author

@jtbandes jtbandes Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If I pass an invalid reason, including an empty string, I get 422 Unprocessable Entity:

{
  "message": "Please specify a valid state reason.",
  "documentation_url": "https://docs.github.com/rest/reference/issues#update-an-issue"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok, I think in that case, it makes sense to check if it's one of the 2 valid values. If not, just fallback to the default. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, it just means that if GitHub introduces a new value in the future, we'll have to push another update to this action.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, understood. IMHO that's preferable to potentially breaking the action in a potentially confusing way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luketomlinson changes made, please take another look when you get a chance. Thanks!


### List of output options

Expand Down Expand Up @@ -509,6 +510,12 @@ Useful to override [ignore-updates](#ignore-updates) but only to ignore the upda

Default value: unset

#### close-as-not-planned

When closing issues, close them as ["not planned"](https://github.blog/changelog/2022-05-19-the-new-github-issues-may-19th-update/).

Default value: unset

### Usage

See also [action.yml](./action.yml) for a comprehensive list of all the options.
Expand Down
3 changes: 2 additions & 1 deletion __tests__/constants/default-processor-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,6 @@ export const DefaultProcessorOptions: IIssuesProcessorOptions = Object.freeze({
ignoreUpdates: false,
ignoreIssueUpdates: undefined,
ignorePrUpdates: undefined,
exemptDraftPr: false
exemptDraftPr: false,
closeAsNotPlanned: undefined
});
3 changes: 2 additions & 1 deletion src/classes/issue.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ describe('Issue', (): void => {
ignoreUpdates: false,
ignoreIssueUpdates: undefined,
ignorePrUpdates: undefined,
exemptDraftPr: false
exemptDraftPr: false,
closeAsNotPlanned: undefined
};
issueInterface = {
title: 'dummy-title',
Expand Down
5 changes: 4 additions & 1 deletion src/classes/issues-processor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,10 @@ export class IssuesProcessor {
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: issue.number,
state: 'closed'
state: 'closed',
state_reason: this.options.closeAsNotPlanned
? 'not_planned'
: undefined
});
}
} catch (error) {
Expand Down
3 changes: 2 additions & 1 deletion src/enums/option.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,6 @@ export enum Option {
IgnoreUpdates = 'ignore-updates',
IgnoreIssueUpdates = 'ignore-issue-updates',
IgnorePrUpdates = 'ignore-pr-updates',
ExemptDraftPr = 'exempt-draft-pr'
ExemptDraftPr = 'exempt-draft-pr',
CloseAsNotPlanned = 'close-as-not-planned'
}
1 change: 1 addition & 0 deletions src/interfaces/issues-processor-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,5 @@ export interface IIssuesProcessorOptions {
ignoreIssueUpdates: boolean | undefined;
ignorePrUpdates: boolean | undefined;
exemptDraftPr: boolean;
closeAsNotPlanned: boolean | undefined;
}
3 changes: 2 additions & 1 deletion src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ function _getAndValidateArgs(): IIssuesProcessorOptions {
ignoreUpdates: core.getInput('ignore-updates') === 'true',
ignoreIssueUpdates: _toOptionalBoolean('ignore-issue-updates'),
ignorePrUpdates: _toOptionalBoolean('ignore-pr-updates'),
exemptDraftPr: core.getInput('exempt-draft-pr') === 'true'
exemptDraftPr: core.getInput('exempt-draft-pr') === 'true',
closeAsNotPlanned: _toOptionalBoolean('close-as-not-planned')
};

for (const numberInput of [
Expand Down