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

temporarily remove auto-merging of Dependabot patches #109

Closed
echappen opened this issue Oct 31, 2023 · 9 comments · Fixed by #215
Closed

temporarily remove auto-merging of Dependabot patches #109

echappen opened this issue Oct 31, 2023 · 9 comments · Fixed by #215
Assignees

Comments

@echappen
Copy link
Contributor

echappen commented Oct 31, 2023

Per this decision, we will auto-merge Dependabot patches.

Per the discussion below, we will remove auto-merging temporarily while we figure out the errors around it in #211

@echappen
Copy link
Contributor Author

echappen commented Nov 7, 2023

PR #139 implements this. I will keep watching this to make sure it works as intended.

igorkorenfeld pushed a commit that referenced this issue Nov 7, 2023
@echappen
Copy link
Contributor Author

echappen commented Nov 9, 2023

FYI: I'm leaving this issue under "In Review" for now while I monitor Dependabot PRs to check that this is working as intended. Once I get confirmation, I'll move this to Done.

@echappen
Copy link
Contributor Author

I’ve been monitoring how this has been working in practice.

We have a number of (necessary and good) constraints on our git workflow that complicates how auto-merging works:

  1. We require all checks to pass before we can merge
  2. We require at least 1 approving reviewer before merging
  3. We require all PR’s to be up-to-date before merging

I’ve made a few tweaks to the GH workflow to ensure proper order of operations:

  • To accommodate #1, I added a conditional to ensure that all other CI checks pass before this particular workflow is run.
  • To accommodate #2, I added a step in the workflow to automatically approve the PR right before merging it (after confirming that we do want to merge it).

With these tweaks, I will need to continue monitoring how this works in practice. Since #3 was enabled recently, I’m still not sure how this will work in combination with the GH workflow.

The happy path I anticipate is:

  1. Dependabot PR’s open in bulk, once per week, each off of the most up-to-date-code
  2. If a single patch update is included in that bulk, then once the CI checks pass for that PR, it will be “approved” and merged automatically

Based on behavior I’ve seen so far, I doubt multiple patch updates will be auto-merged at once, because if one patch update gets merged, the other patch update will become out-of-date and blocked from merging.

We may figure out from all of this that the juice may not be worth the squeeze, and this just creates more work for maintainers, which is what we're trying to reduce.

@echappen
Copy link
Contributor Author

@alexsobledotgov is going to keep monitoring Dependabot PR's for me while I'm away Thanksgiving week. Thanks!

@jskinne3
Copy link
Contributor

jskinne3 commented Nov 20, 2023

Today three pull requests failed auto-merge with the error:

failed to run git: fatal: not a git repository (or any of the parent directories): .git

Hypotheses:

  1. An actions/checkout is needed in order to switch to the project directory, or
  2. Maybe this issue currently open in GitHub Actions

@echappen
Copy link
Contributor Author

These failures cause the CI pipeline to fail, which prevents merging of these patch PR’s. So now it’s blocking work more than usual.

If someone has a known/quick fix for this error, then I could see us continuing. If not, I suggest removing the dependabot auto-merge job from the pipeline, and giving up on the dream of having patches auto-merge.

I’ll leave it up to the TLC leads to decide whether it’s worth the time to attempt fixing. @alexbielen @juliaklindpaintner

@alexbielen
Copy link

Let's remove the auto-merge for now -- I'll create a ticket to follow up, but let's eliminate it as a blocker @echappen

@alexbielen
Copy link

@echappen FYSA #211

@echappen
Copy link
Contributor Author

We'll use this issue to handle removing the auto-merging while #211 is being worked on.

@echappen echappen changed the title Set up auto-merging of Dependabot patches temporarily remove auto-merging of Dependabot patches Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants