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

[infra] Survey closed issues #11805

Merged
merged 6 commits into from
Feb 8, 2024
Merged

[infra] Survey closed issues #11805

merged 6 commits into from
Feb 8, 2024

Conversation

michelengelen
Copy link
Member

adds an action to add a comment on closed issues and also changes the closeComment text when auto closing issues for inactivity

@michelengelen michelengelen added core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product labels Jan 24, 2024
@michelengelen michelengelen self-assigned this Jan 24, 2024
@mui-bot
Copy link

mui-bot commented Jan 24, 2024 •
edited
Loading

Deploy preview: https://deploy-preview-11805--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 6e3bcf3

joserodolfofreitas
joserodolfofreitas previously approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

Nice work!👌

LukasTy
LukasTy reviewed Feb 7, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice work! 👍
A couple of nitpicks:

  1. This will produce a couple of github-actions comments in cases when an inactive issue has been closed. Have you considered any approach to avoid this? Or you don't see it as a problem? 🤔
  2. Weren't we discussing a solution of adding a comment only in case a user comments on a stale (X amount of time) issue to reduce the "noise"?
    Have you considered/explored anything on that regard? 🤔

.github/workflows/closed-issue-message.yaml Outdated Show resolved Hide resolved
@joserodolfofreitas joserodolfofreitas self-requested a review February 7, 2024 18:45
@joserodolfofreitas joserodolfofreitas dismissed their stale review February 7, 2024 18:46

to re-review after adding call to survey

@michelengelen
Copy link
Member Author

  1. This will produce a couple of github-actions comments in cases when an inactive issue has been closed. Have you considered any approach to avoid this? Or you don't see it as a problem? 🤔

Yes, there is a way. And it's relatively easy as well we just have to ask @MBilalShafi to add a state_reason: 'inactivity' to this line: https://github.com/MBilalShafi/no-response-add-label/blob/629add01d7b6f8e120811f978c42703736098947/src/no-response.ts#L151C36-L151C42

Then we can filter issues that have this reason to not add the comment in my action. It gets returned by the action webhook under github.event.issue.state_reason

  1. Weren't we discussing a solution of adding a comment only in case a user comments on a stale (X amount of time) issue to reduce the "noise"?

No, we discussed this for every closed issue.

For a simple reason: It's an additional step and can lead to misunderstandings if we only add this when users comment on closed issues. Just think about a case where someone comments and immediately closes. He might miss the comment. And when he comments and reads the message he has to go the extra step of creating a new issue with the information he just entered.

IMHO it is the better approach to preemptively add the comment.

Note: we can add a link to the survey for the author of the ticket as well. 👍🏼

@michelengelen
added an if to check for the state reason and a condition to only sho…
60029c8 
…w the questionaire for non members/owners of mui repos
@michelengelen
Merge remote-tracking branch 'origin/automation/close-message' into a…
7a2a4ca 
…utomation/close-message

# Conflicts:
#	.github/workflows/closed-issue-message.yaml
@michelengelen
Copy link
Member Author

@LukasTy I did add the guard for the state_reason change already, which means for now it would run on every closed issue until the change is made on the other action.

I did also add the line to our survey, but just for issues that have been opened from outside of the org!

LukasTy
LukasTy approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice improvements and initiative! 👏
If everyone else is onboard with this change, let's go for it. 👍
We can always adjust if need be. 👌

joserodolfofreitas
joserodolfofreitas approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

Awesome!
I'm excited to see some answers for the recently added survey.
Great work!

joserodolfofreitas
joserodolfofreitas reviewed Feb 8, 2024
View reviewed changes
.github/workflows/closed-issue-message.yaml Outdated
If you have a similar problem, please open a [new issue](https://github.com/mui/mui-x/issues/new/choose) and provide details about your specific problem.
If you can provide additional information related to this topic that could help future readers, please feel free to leave a comment.
${{ !['MEMBER', 'OWNER'].includes(github.event.issue.author_association) ? '
**How did we do @${{ github.event.issue.user.login }}?**
Copy link
Member

Choose a reason for hiding this comment

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

👌

@michelengelen michelengelen enabled auto-merge (squash) February 8, 2024 10:56
@michelengelen michelengelen merged commit 4ab9d44 into mui:next Feb 8, 2024
15 checks passed
@michelengelen michelengelen added the needs cherry-pick The PR should be cherry-picked to master after merge label Feb 8, 2024
@michelengelen michelengelen deleted the automation/close-message branch February 8, 2024 13:08
michelengelen added a commit to michelengelen/mui-x that referenced this pull request Feb 8, 2024
@michelengelen @LukasTy
[code-infra] auto-message on closed issues (mui#11805)
a3f3d64 
Signed-off-by: Michel Engelen <[email protected]>
Co-authored-by: Lukas <[email protected]>
@michelengelen michelengelen mentioned this pull request Feb 8, 2024
Closed
1 task
@oliviertassinari oliviertassinari removed the needs cherry-pick The PR should be cherry-picked to master after merge label Feb 11, 2024
@oliviertassinari oliviertassinari changed the title [code-infra] auto-message on closed issues [infra] auto-message on closed issues Feb 11, 2024
@oliviertassinari oliviertassinari added scope: infra Org infrastructure work going on behind the scenes and removed core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product labels Feb 11, 2024
@oliviertassinari oliviertassinari changed the title [infra] auto-message on closed issues [infra] Auto-message on closed issues Feb 11, 2024
@oliviertassinari oliviertassinari changed the title [infra] Auto-message on closed issues [infra] Survey closed issues Feb 11, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2024 •
edited
Loading

Nice 👍

Could we restrict permissions at the top https://github.com/mui/mui-x/security/code-scanning/31? Thanks

This was referenced Feb 22, 2024
Merged
Merged
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
@michelengelen @LukasTy @thomasmoon
[code-infra] auto-message on closed issues (mui#11805)
bf65ef9 
Signed-off-by: Michel Engelen <[email protected]>
Co-authored-by: Lukas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaviendelangle flaviendelangle flaviendelangle left review comments

@joserodolfofreitas joserodolfofreitas joserodolfofreitas approved these changes

@LukasTy LukasTy LukasTy approved these changes

@DanailH DanailH Awaiting requested review from DanailH

@MBilalShafi MBilalShafi Awaiting requested review from MBilalShafi

Assignees

@michelengelen michelengelen

Labels
scope: infra Org infrastructure work going on behind the scenes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michelengelen @mui-bot @oliviertassinari @joserodolfofreitas @flaviendelangle @LukasTy