-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add workflow job to deduplicate dependabot pull requests #14845
Conversation
I assume the double CI checks that are pending are just a GitHub quirk that will correct itself after merging? |
@bramkragten what are your thoughts on this one? I've been committing the deduplications manually to keep the ball rolling, but would be nice to automate. |
.github/workflows/ci.yaml
Outdated
jobs: | ||
dedupe: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this even needed?
Why is this not solved by dependabot or even yarn?
I find it strange that every project that relies on dependabot would need some workaround like this?
This also breaks the auto rebase feature that dependabot does as it refuses to change a PR that someone else has messed with, which means that we would need to comment @dependabot recreate
for all PRs.
This is not great from a resource perspective (we should also add concurrency with cancel to the CI workflow but that's a separate topic.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this even needed? Why is this not solved by dependabot or even yarn?
Yarn certainly can't control what dependabot does, but yeah I totally agree dependabot should handle it. Hopefully this would be temporary. Here's the issue: dependabot/dependabot-core#5830
This also breaks the auto rebase feature that dependabot does as it refuses to change a PR that someone else has messed with, which means that we would need to comment
@dependabot recreate
for all PRs.
Yeah I agree, although not "all" PRs will need the deduplication step. Theoretically, there would be less and less commits required as dependencies are brought up to date.
This is not great from a resource perspective (we should also add concurrency with cancel to the CI workflow but that's a separate topic.)
I was going to add that but didn't want to go off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the direct need for it to run and push in this workflow at all.
We are trying to do all kinds of weird things to make this happen. IMHO we can just make this a maintenance task, or let a task run every day or week on a schedule to clean this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing CI checks for duplicates via yarn dedupe --check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If thats all that this is really for, we can just remove that check and do this as general maintenance and/or on a separate schedule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's wise. The check is run to minimize what packages get bundled and sent to the browser. Leaving duplicates can affect frontend results. It's not just maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make a separate action, that checks if a dedupe is needed and creates a PR if it is and run that daily, it might be an easier fix? Or even run it after a merge of a dependabot PR.
We can still have the dedupe check in CI for manual PRs and skip it for dependabot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally wouldn't be a fan of that because you'd be degrading any validation and testing on the original bump, only to make more work for yourself on the subsequent deduplication. It would also be arguably more work than this change, especially to automate opening the PR.
I honestly didn't expect push back over "skipped" messages, but since this is getting no love, what's the problem with making a separate workflow (only to run on dependabot branches) and using a GitHub app to authenticate?
I could setup a new app and transfer ownership to HA (or use an existing bot?). Then use a 3rd party action like this one to authenticate and commit as the app. Someone above my pay grade just needs to add the credentials as secrets.
The only downside to that approach is that if the app commits, then CI runs twice and wastes resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example from a recent PR to support keeping the check (and moving it up before the others):
https://github.com/home-assistant/frontend/actions/runs/4022152029
The CI failed during the types check, but the actual problem was the imports getting mangled with a duplicate. Simply running the dedupe was the fix.
app_id: ${{ secrets.HA_COMMITTER_APP_ID }} | ||
private_key: ${{ secrets.HA_COMMITTER_PRIVATE_KEY }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@balloob I'm about to transfer ownership of a simple GH app with content permissions. Could you:
- Add a couple secrets for the App ID (not client ID) and private key with these names? You'll have to generate a new private key and delete the existing one.
- Install the app in the frontend repo?
The revised version I pushed earlier now largely leaves the existing CI as is. It runs a separate workflow only on dependabot branches and only when dependabot pushes. I have it as a draft until I can test the installed app authentication, but please feel free to review. |
@@ -0,0 +1,54 @@ | |||
name: Deduplicate Dependabot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with this being split out in a separate workflow, I still do not think this is something we should add.
If this is an issue, that should be documented and reported to GitHub.
We should not add this level of workarounds, that to me, at least clearly show that something is wrong with how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @ludeeus on this.
As an alternative could be: Don't use dependabot for this task.
An alternative could be, for example, Renovate. That is a competitor to dependabot that can actually dedupe (if configured as such).
../Frenck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renovate seems like a nice option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree Renovate is probably the better option for HA. In addition to being able to dedupe, it also handles monorepos much better, which has also been annoying from dependabot.
Closing this, but for posterity bringing it up to date and also added a simple change which addresses @ludeeus's concern about dependabot losing its rebase capability when committing the dedupe.
I'll submit a request to install Renovate and would be happy to work on the config PR.
Proposed change
Many dependabot pull request will fail the duplicates check in the CI workflow. This change automatically runs
yarn dedupe
on dependabot pull requests, makes a commit by the actions bot using the GITHUB_TOKEN for the workflow, and then continues the checks with the update.Here are final successful test runs, and you can see the successful commits in the history of #14775.
Note that actions taken with the
GITHUB_TOKEN
never trigger new workflows, so this is designed as part of the current CI workflow and manually deals with the correct commit SHA to continue the CI. The alternative would be to make it a separate workflow that uses a PAT for the commit, but that obviously has security implications and also wastes resources because the CI will start and then be re-triggered shortly after.Other minor changes:
permissions: {}
syntax for clarityCI
environment variable (this is automatically true for every workflow)Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: