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.

Sign up for GitHub

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

Jump to bottom

Add workflow job to deduplicate dependabot pull requests #14845

Closed
wants to merge 9 commits into from
Closed

Add workflow job to deduplicate dependabot pull requests #14845

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
49e2230
Add workflow job to deduplicate dependabot pull requests
steverep Dec 20, 2022
9974559
Update bash style to quote variables and use [[ and $()
steverep Dec 21, 2022
555c43c
Merge dev and pin action versions
steverep Dec 27, 2022
2fdb6f1
Merge dev to bump action versions
steverep Jan 9, 2023
de03c96
Revise to use separate workflow with GitHub app
steverep Feb 6, 2023
7c1b2e0
Add concurrency rules to cancel workflows in progress
steverep Feb 6, 2023
bb6fb5f
Move dedupe check up to fail faster
steverep Feb 6, 2023
6ba567c
Merge branch 'dev' into dependabot-dedupe
steverep Feb 14, 2023
f7e2fb5
Allow dependabot to rebase over dedupe commit
steverep Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
99 changes: 85 additions & 14 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,82 @@ env:
NODE_OPTIONS: --max_old_space_size=6144
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

permissions: {}
steverep marked this conversation as resolved.
Show resolved Hide resolved

jobs:
dedupe:
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

name: Deduplicate dependencies
# Skip unless this is a dependabot pull request
if: |
github.actor == 'dependabot[bot]' &&
startsWith(github.head_ref, 'dependabot/npm_and_yarn/')
steverep marked this conversation as resolved.
Show resolved Hide resolved
permissions:
contents: write
runs-on: ubuntu-latest
outputs:
# Downstream jobs need to use this SHA to get the dedupe commit
sha: ${{ steps.get-sha.outputs.sha }}
steps:
- name: Check out files from GitHub
uses: actions/checkout@v3
steverep marked this conversation as resolved.
Show resolved Hide resolved
with:
# Checkout PR head instead of merge commit
# Use ref, not SHA, so reruns get the dedupe commit
ref: ${{ github.event.pull_request.head.ref }}
- name: Set up Node ${{ env.NODE_VERSION }}
uses: actions/setup-node@v3
steverep marked this conversation as resolved.
Show resolved Hide resolved
with:
node-version: ${{ env.NODE_VERSION }}
cache: yarn
- name: Install dependencies
# Do not run build scripts as a security measure since job has write permissions
run: yarn install --immutable --mode=skip-build
- name: Deduplicate dependencies
run: yarn dedupe --mode=skip-build
- name: Commit changes
run: |
git config user.name "GitHub Action"
git config user.email "github-action@users.noreply.github.com"
git add yarn.lock
git commit -m "Deduplicate dependencies" || exit 0
git push origin HEAD:$GITHUB_HEAD_REF
steverep marked this conversation as resolved.
Show resolved Hide resolved
echo "DEDUPED=true" >> $GITHUB_ENV
steverep marked this conversation as resolved.
Show resolved Hide resolved
- name: Output updated SHA for merge commit
id: get-sha
shell: bash
timeout-minutes: 15
run: |
if [ -v DEDUPED ]; then
steverep marked this conversation as resolved.
Show resolved Hide resolved
echo "Waiting for GitHub to do the mergability check and update the commit SHA..."
while [ -z "$sha" -o "$sha" == "$GITHUB_SHA" ]; do
sleep 5s
sha=`git ls-remote origin $GITHUB_REF | awk '{print $1}'`
done
steverep marked this conversation as resolved.
Show resolved Hide resolved
else
echo "No deduplication required so using current merge commit SHA"
# Still need to query remote here in case of rerun where previous attempt was deduplicated
sha=`git ls-remote origin $GITHUB_REF | awk '{print $1}'`
steverep marked this conversation as resolved.
Show resolved Hide resolved
fi
echo "Done - SHA is $sha"
steverep marked this conversation as resolved.
Show resolved Hide resolved
echo "sha=$sha" >> $GITHUB_OUTPUT
lint:
name: Lint and check format
needs: dedupe
# Allow dedupe job to be skipped
if: ${{ !failure() && !cancelled() }}
runs-on: ubuntu-latest
steps:
- name: Check out files from GitHub
uses: actions/checkout@v3
with:
ref: ${{ needs.dedupe.outputs.sha }}
- name: Set up Node ${{ env.NODE_VERSION }}
uses: actions/setup-node@v3
with:
node-version: ${{ env.NODE_VERSION }}
cache: yarn
- name: Install dependencies
run: yarn install
env:
CI: true
run: yarn install --immutable
- name: Build resources
run: ./node_modules/.bin/gulp gen-icons-json build-translations build-locale-data gather-gallery-pages
- name: Run eslint
Expand All @@ -41,57 +102,67 @@ jobs:
- name: Check for duplicate dependencies
run: yarn dedupe --check
test:
name: Run tests
needs: dedupe
# Allow dedupe job to be skipped
if: ${{ !failure() && !cancelled() }}
runs-on: ubuntu-latest
steps:
- name: Check out files from GitHub
uses: actions/checkout@v3
with:
ref: ${{ needs.dedupe.outputs.sha }}
- name: Set up Node ${{ env.NODE_VERSION }}
uses: actions/setup-node@v3
with:
node-version: ${{ env.NODE_VERSION }}
cache: yarn
- name: Install dependencies
run: yarn install
env:
CI: true
run: yarn install --immutable
- name: Build resources
run: ./node_modules/.bin/gulp build-translations build-locale-data
- name: Run Tests
run: yarn run test
build:
name: Build frontend
needs: [dedupe, lint, test]
# Allow dedupe job to be skipped
if: ${{ !failure() && !cancelled() }}
runs-on: ubuntu-latest
needs: [lint, test]
steps:
- name: Check out files from GitHub
uses: actions/checkout@v3
with:
ref: ${{ needs.dedupe.outputs.sha }}
- name: Set up Node ${{ env.NODE_VERSION }}
uses: actions/setup-node@v3
with:
node-version: ${{ env.NODE_VERSION }}
cache: yarn
- name: Install dependencies
run: yarn install
env:
CI: true
run: yarn install --immutable
- name: Build Application
run: ./node_modules/.bin/gulp build-app
env:
IS_TEST: "true"
supervisor:
runs-on: ubuntu-latest
name: Build supervisor
needs: [lint, test]
# Allow dedupe job to be skipped
if: ${{ !failure() && !cancelled() }}
runs-on: ubuntu-latest
steps:
- name: Check out files from GitHub
uses: actions/checkout@v3
with:
ref: ${{ needs.dedupe.outputs.sha }}
- name: Set up Node ${{ env.NODE_VERSION }}
uses: actions/setup-node@v3
with:
node-version: ${{ env.NODE_VERSION }}
cache: yarn
- name: Install dependencies
run: yarn install
env:
CI: true
run: yarn install --immutable
- name: Build Application
run: ./node_modules/.bin/gulp build-hassio
env:
Expand Down