search

hashicorp / terraform-provider-tls

Utility provider that works with Transport Layer Security keys and certificates. It provides resources that allow private keys, certificates and certficate requests to be created as part of a Terraform deployment.
https://registry.terraform.io/providers/hashicorp/tls/latest
Mozilla Public License 2.0
184 stars 102 forks source link

Don't leave comment when locking merged PR #525

Open anuraaga opened 4 months ago

anuraaga commented 4 months ago

Terraform CLI and Provider Versions

n/a

Terraform Configuration

n/a

Expected Behavior

Merged PRs don't cause notifications

Actual Behavior

Got a notification for my merged PR being locked

https://github.com/hashicorp/terraform-provider-tls/pull/34#issuecomment-2139074344

Steps to Reproduce

Have a PR in the repo from 30+ days ago.

How much impact is this issue causing?

High

Logs

No response

Additional Information

I'm not sure if this is the correct repo for this since I suspect it's an org-wide setting, but if it's possible it would be good to reconfigure this lockbot to only do it for open PRs. Otherwise, it causes unnecessary spam to contributors.

Code of Conduct

bflad commented 4 months ago

Hi @anuraaga 👋 Thank you for raising this and sorry for the potentially frustrating notification.

One coincidental thing here is that a GitHub rate limiting issue was resolved with the locking process which over the last few days has caused all the notifications and locking to actually occur when it potentially hasn't in the past. It looks like the locking process is now "caught up" so I would not expect any more bulk notifications.

The current setup for issue and PR locking is for the automation to add the "I'm going to lock this issue" comment, then lock it, when issues and PRs have been closed for 30+ days. We would not want to start locking open issues and PRs as it would prevent any further issue or PR discussion to occur except from the maintainers. The maintainers will only lock open issues or PRs if there is an urgent need.

As for the comment part of the process (the source of the notification), it was previously added to the locking process because practitioners in the past have been confused that the locking was due to some passage of time and showed that frustration when creating new issues if there were unfinished conversations that happened on the original issue. Folks understandably get upset if they do not feel like they are being heard. GitHub only offers lock reasons of "Off-topic", "Too heated", "Resolved", and "Spam". "Resolved" as a potentially silent (from a notification standpoint) reason is mostly correct, except if folks were trying to continue a discussion on a given issue or PR.

I think the maintainers would be amenable to potentially dropping the lock comment/notification if there is enough community interest though, so I will keep this issue open for further discussion from you, the maintainers, and the community.

anuraaga commented 4 months ago

Thanks @bflad - I had assumed the locking was to prevent stale, open PRs from piling up but realize it was explicitly for closed PRs. I guess the motivation to lock a merged PR is to prevent people from commenting on very old PRs instead of filing a new issue, makes sense.

Indeed I guess the comment, which causes a notification, is the main issue here. I'd personally not be upset at a merged PR being locked silently after some time has passed since a merged PR is done, but not sure how generally people would take it. A way to have a notification that doesn't bring old PRs back into inboxes could be to leave something including "We will lock this PR in xx days" automatically as a comment when the PR is merged, rather than when the PR is locked. That timing is still when the PR is fresh in an inbox, while also leaving the comment trail about the locking policy.

Note that the policy may deserve to be a little different for issues vs PRs, especially PRs merged rather than being rejected.

Anyways, just some ideas, thanks for the consideration.