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.

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

[Repository automation] Flatten hierarchy for merge rights #36492

Open
ArthurSens opened this issue Nov 21, 2024 · 10 comments
Open

[Repository automation] Flatten hierarchy for merge rights #36492

ArthurSens opened this issue Nov 21, 2024 · 10 comments
Labels
ci-cd CI, CD, testing, build issues

Comments

@ArthurSens
Copy link
Member

ArthurSens commented Nov 21, 2024

Problem statement

When an external contributor works on a PR, this is the current process:

  • Contributor and reviewers work together on a PR.
  • Codeowner approves PR.
  • Collector approver adds "Ready to merge" label.
  • Collector maintainers go over PRs with the "Ready to merge" label and merge them or remove the label if a new conflict has appeared.

A symptom of this process is that code owners lack the independence to unblock community contributions. Collector approvers/maintainers are a smaller group than component code owners but have more responsibilities within the OpenTelemetry community.

Since the group of people with merge rights is small and we have a multi-layer process to let maintainers know when things should be merged, it's pretty common that PRs that are ready stay open for weeks or even months without people realizing that it's ready to merge.

This leads to a bad experience for multiple parties:

  • Contributor - who has to maintain an open PR for too long, often resolving merge conflicts several times and having to "hunt" approvers and maintainers.
  • Code owners - who feel impotent in helping the contributor besides pinging people over GitHub/Slack.
  • Approvers/Maintainers - who are the busiest people and are constantly pinged with "please merge my PR" only to find a merge conflict.

Suggested Improvements

The blame doesn't lie with any particular individual; everyone is doing what they were supposed to do. The poor contributor experience here is a symptom of a vertical hierarchy. The solution is to give more power to the lower levels (code owners) and free maintainers to do more relevant work.

Things we could do:

  • Empower code owners to merge PRs related to their components.
    or
  • Empower collector approvers to merge PRs.
    or
  • Automate the addition of the "Ready to merge" label so we can speed up the process by removing one step.

How

This is what I'd like to discuss with the community 🙂

To what degree of freedom are the maintainers happy to give code owners?
What kind of automation needs to be built to unblock the freedom we're discussing?

@ArthurSens ArthurSens added the needs triage New item requiring triage label Nov 21, 2024
@ArthurSens ArthurSens changed the title [Repository automation] Automate addition of "Ready to merge" label [Repository automation] Flatten hierarchy for merge rights Nov 21, 2024
@mx-psi
Copy link
Member

mx-psi commented Nov 22, 2024

To me the two low hanging fruit items that would help here:

  • Add a merge queue. In my experience as a maintianer the main reason I don't merge PRs even if they are ready is because the tests are not ready. If we had a merge queue, I wouldn't have to wait for that: as long as tests pass things are fine. This needs some trust in contributors but would be a huge improvement. We tried this once REQUEST: Repository maintenance on opentelemetry-collector-contrib community#1936, we had some issues that I think should be solved now, but I would rather start adding the merge queue to opentelemetry-collector-releases first.
  • The 'Ready to merge' automation you mention.

I am a bit hesitant about the other suggestions, the definitions at the community repo say that only maintainers "[Decide] on when PRs are merged to control the release scope." We can clarify this on the community repository if this is the direction we want to take, but I feel like we should first try to solve this while abiding by the existing project-wide approach.

I also feel like we should measure if these have any noticeable improvement. We can use these two dashboards for this:

@ArthurSens
Copy link
Member Author

ArthurSens commented Nov 22, 2024

We tried this once open-telemetry/community#1936, we had some issues that I think should be solved now, but I would rather start adding the merge queue to opentelemetry-collector-releases first.

After reading the issue you mentioned, it looks like there are no blockers now. Is there anything I could do to help test this in a smaller repository?

The 'Ready to merge' automation you mention.

What are the requirements for adding the label, in your opinion? Things that came to my mind:

  • Code owner approves the PR + CI is green.
  • Code owner approves the PR + CI is green + all conversations resolved.

If any of the requirements aren't met, the automation should also be able to remove the label.

I am a bit hesitant about the other suggestions, the definitions at the community repo say that only maintainers "[Decide] on when PRs are merged to control the release scope."

I feel like this definition works great for all repositories except Contrib due to its large pool of code and contributors. Of course, I can't speak for all folks, but my experience as a contributor/code owner is not the best because my PRs/PRs I approve stay open for way too long. I think I've solved more merge conflicts in this repo than the sum of all the other repositories I work on today.

I'm not entirely familiar with the process here in OTel (I've been contributing for only a few months), do we need to propose changes to the community repository before making changes here? Or are we free to work on improvements and change the guidelines later? I don't want to overstep whatever process is in place today.

With that said, let's work on the low hanging fruits for now and worry about the big changes if we feel like improvements still need to be made :)

I also feel like we should measure if these have any noticeable improvement. We can use these two dashboards for this:

Totally agree! Another thing I believe is worth measuring is "Contributor Happiness". I'm not sure how that could be measured though... Maybe the number of contributors that keep contributing after the first PR?

@mx-psi
Copy link
Member

mx-psi commented Nov 25, 2024

Code owner approves the PR + CI is green.

This is the definition we have right now + human judgement. To sidestep the human judgement bit, maybe we can add if code owner approval + green CI + all convos resolved and remove if CI is not green.

I think I've solved more merge conflicts in this repo than the sum of all the other repositories I work on today.

Maybe this is another thing that we can develop better tooling for?

do we need to propose changes to the community repository before making changes here?

As long as there is no contradiction with what the community guidelines say we can set our own guidance/goals/etc. But I think adding right to merge to people that are not maintainer is probably controversial enough to warrant discussion at the community repository 😄

Totally agree! Another thing I believe is worth measuring is "Contributor Happiness". I'm not sure how that could be measured though... Maybe the number of contributors that keep contributing after the first PR?

I am working on an OTel-wide contributor survey which is available here: https://docs.google.com/document/d/1YlwHJlJjuaR4i8VqWf1eIOpT158TOHdLii8iC0ndJXc/edit?tab=t.0 There is also open-telemetry/sig-contributor-experience/issues/28 that can help here.

@ArthurSens
Copy link
Member Author

This is the definition we have right now + human judgement. To sidestep the human judgement bit, maybe we can add if code owner approval + green CI + all convos resolved and remove if CI is not green.

Sounds good to me

Maybe this is another thing that we can develop better tooling for?

One thing that could help here is a bot that adds a comment to the PR, tagging the author when new conflicts appear. Unfortunately, GitHub doesn't notify people of merge conflicts, so it usually goes unnoticed for a long time.

As long as there is no contradiction with what the community guidelines say we can set our own guidance/goals/etc. But I think adding right to merge to people that are not maintainer is probably controversial enough to warrant discussion at the community repository 😄

Fair, I'll wait a bit to ping on this idea again. But one day, code owners will have their freedom!!!!!!! 😝

I am working on an OTel-wide contributor survey which is available here: https://docs.google.com/document/d/1YlwHJlJjuaR4i8VqWf1eIOpT158TOHdLii8iC0ndJXc/edit?tab=t.0 There is also open-telemetry/sig-contributor-experience/issues/28 that can help here.

Awesome, I'll try to help there

@evan-bradley evan-bradley added ci-cd CI, CD, testing, build issues and removed needs triage New item requiring triage labels Nov 26, 2024
@ArthurSens
Copy link
Member Author

Interesting. While doing some research about building some sort of notification automation for merge conflicts I discovered this:

image

That is a big improvement for me but it's such a hidden configuration option lol, not sure if everyone is aware... Should we still invest time and effort in the automation anyway?

@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2024

I think there is less value on the merge conflict part then. But still, the 'ready to merge' automation is nice to have

@Aneurysm9
Copy link
Member

I feel like this definition works great for all repositories except Contrib due to its large pool of code and contributors. Of course, I can't speak for all folks, but my experience as a contributor/code owner is not the best because my PRs/PRs I approve stay open for way too long. I think I've solved more merge conflicts in this repo than the sum of all the other repositories I work on today.

I feel that this may be an indication that the collector-contrib repo is too big for its own good. Ultimately, I think this comes down to the collector core framework not being stabilized such that the best (only?) way to remain up-to-date is to have your component in the collector-contrib repo such that maintainers handle mechanistic breaking changes and dependency updates for you. This shouldn't be necessary and there should be many more collector components that live outside of this repo and even outside the open-telemetry organization.

Having a merge queue and automation for ready to merge sound great, but changes to who has the authority to merge PRs would be a significant departure from the longstanding policy and practice of this community across every repository in the organization. I would prefer that we seek to fix the underlying problem rather than blur the lines of responsibility or go chasing after a suitable automated merge system that probably doesn't exist.

@mx-psi
Copy link
Member

mx-psi commented Nov 27, 2024

Ultimately, I think this comes down to the collector core framework not being stabilized such that the best (only?) way to remain up-to-date is to have your component in the collector-contrib repo such that maintainers handle mechanistic breaking changes and dependency updates for you.

Agree with this but this will take time still. I am more than happy to get help on the Collector v1 effort to make it happen faster if there is interest 😄

@mx-psi
Copy link
Member

mx-psi commented Dec 11, 2024

Filed #36788 to track the remaining steps for the merge queue. @ArthurSens would you like to help with any of those?

@ArthurSens
Copy link
Member Author

Sure thing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cd CI, CD, testing, build issues
Projects
None yet
Development

No branches or pull requests

4 participants