Helm-charts: Merging Process

[torstenwalter]

I noticed that several people approved #51 but no one merged it. What should our process for this look like?

1. The one who created the PR is in charge for merging
   I think that's good for PRs like #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..
2. The one who reviews the PR also merges it
   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.
3. Implement auto merge based on criteria e.g.
   
   
   
   • PR has at least one approved review
   
   
   • all required status checks are passing (chart linting, superlinter, DCO)
     
     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?

[scottrigby]

@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.

[scottrigby]

Personally I'd leave PR merge criteria into mainas we have it now:

• at least one pull request approval
• can only be merged by a user defined in the CODEOWNERS file (based on which files/directories are modified in the PR)

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:

When a pull request is merged by this action, the merge will not trigger other GitHub workflows

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.

[torstenwalter]

Haven't seen that limitation.

[scottrigby]

@torstenwalter WDYT about my reasoning for leaving option 1 or 2 up for human judgement?

[torstenwalter]

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.

[scottrigby]

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?

[monotek]

I vote for 3.
Not sure if i would prefer 2 reviews though.

[rsotnychenko]

For option 3, if we wanted it I don't think the automerge action will work for us because of this limitation:

When a pull request is merged by this action, the merge will not trigger other GitHub workflows

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.

@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 env.GITHUB_TOKEN. The token can be securely stored in Repository Secrets, but keep it mind that anyone with write access to the repo could potentially read it.

[scottrigby]

@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 🙂

[rsotnychenko]

@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!

[torstenwalter]

From the discussion it seems ok to say that we do merges manually for now.
So we should document it in PROGRESS.md file.

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.

[torstenwalter]

We also should document a wait period before an admin steps in as approver in case there is no feedback from other maintainers. See
https://github.com/prometheus-community/helm-charts/pull/68#issuecomment-695337062