-
Notifications
You must be signed in to change notification settings - Fork 5k
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.
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
Merging Process #52
Comments
@torstenwalter I thought the wait on merging #51 was due to this being a top level file so a repo admin should merge it (adding more repo admins being addressed in #38). I left a review comment on your PROCESSES doc PR #44 to add notes about the responsibilities (and limits) of the @prometheus-community/helm-charts-admins team as well. |
Personally I'd leave PR merge criteria into
And beyond that, let who does the merge be a human decision (either 1 or 2 above). There may be PRs where you want feedback from a specific user, but also want to allow other members to review/approve in the meantime. For option 3, if we wanted it I don't think the automerge action will work for us because of this limitation:
In that case, the chart releaser action wouldn't be triggered when merging into main as we need it to release/index the helm repo. |
Haven't seen that limitation. |
@torstenwalter WDYT about my reasoning for leaving option 1 or 2 up for human judgement? |
Totally fine with me. In then end PR approval is also human judgement. I might just be unclear to contributors when something will be merged if something is approved but not merged. |
Makes sense. Maybe in the PROCESSES doc we should clarify the PR merge criteria (the branch protection settings) and suggest that PR approvers should also merge the PR, unless they want more eyes on it by other maintainers or specific people - and if so to leave a comment clarifying who they're requesting feedback from, so everyone including the PR submitter will know the status? |
I vote for 3. |
@scottrigby FYI this limitation can be solved by invoking Merge API using a Personal (from a dedicated bot account) or GitHub App token, instead of the one GitHub provides in |
@rsotnychenko we could do that lol 😉 I agree we can do it, but should we? There are times where you want a specific persons opinion, or more eyes. Even in setting up this repo, we’ve needed opinions from other people on a number of PRs, but for most others 1 CODEOWNERS review was perfect. Auto-closing based on pre-set criteria would have been very annoying. Just to be clear, I’m far from being against automation in general, but this feels like it would get in the way of collaboration rather than helping it. But this is just my opinion, the process has to work for the community of maintainers and contributors as a whole. If you all want it, and it’s done safely, go for it 🙂 |
@scottrigby wasn't saying that we should do it, just giving a heads-up its possible & easy to do 🙂 I don't have a strong opinion whether this automation is needed right now, because I'm kinda new in the org. Will reach out to you and the others once this changes! |
From the discussion it seems ok to say that we do merges manually for now. As a starting point we should document GitHub settings in there so that these are transparent for everyone. Next to that mention that Chart maintainers who approve a PR should also merge it. In case they see a need for further review from others they should state that in a comment. |
We also should document a wait period before an admin steps in as approver in case there is no feedback from other maintainers. See |
Part-of: #52 Signed-off-by: Torsten Walter <mail@torstenwalter.de>
* document repository settings Part-of: #52 Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Align CODEOWNERS with Chart maintainers - Added trailing / to all directories - Use same order of maintainers as in Chart.yaml Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Fixed punctuation and removed double word Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Update PROCESSES.md Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> * Update PROCESSES.md Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> * Update PROCESSES.md Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> * Add monotek as chart maintainer Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Sort maintainers in CODEOWNERS Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Fix sorting of alertmanager codeowners Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com>
* document repository settings Part-of: prometheus-community#52 Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Align CODEOWNERS with Chart maintainers - Added trailing / to all directories - Use same order of maintainers as in Chart.yaml Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Fixed punctuation and removed double word Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Update PROCESSES.md Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> * Update PROCESSES.md Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> * Update PROCESSES.md Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> * Add monotek as chart maintainer Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Sort maintainers in CODEOWNERS Signed-off-by: Torsten Walter <mail@torstenwalter.de> * Fix sorting of alertmanager codeowners Signed-off-by: Torsten Walter <mail@torstenwalter.de> Co-authored-by: Scott Rigby <scott@r6by.com> Signed-off-by: André Bauer <andre.bauer@kiwigrid.com>
…etheus-stack [kube-prometheus-stack] fix missing unscheduled Pods
I noticed that several people approved #51 but no one merged it. What should our process for this look like?
I think that's good for PRs like [prometheus-operator] Rename to kube-prometheus-stack #1, where it's good that multiple persons have a look. The downside is that it only works if the PR creator has write permission for this repository. As we want outside contributions that option can be excluded as a general process..
That's simple and straight forward. Downside is that people have to get back to the PR in case they approve it before status checks have been finished.
That's similar to what we had in stable repository. We would need to investigate how to implement it.
For option 3 we could look into https://github.com/pascalgn/automerge-action
Which option would you prefer?
The text was updated successfully, but these errors were encountered: