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

ci: Apply yarn-deduplicate to dependabot tasks #12234

Merged
merged 9 commits into from Jan 18, 2024

Conversation

Yaminyam
Copy link
Contributor

Motivation

For dependabot pr we are currently accepting and merging automatically.
However, there are cases where the yarn-deduplicate operation is not reflected and cannot be merged automatically.
#12183

Modifications

For dependabot pr, run yarn --cwd ui deduplicate and commit and push the changes.

Verification

Signed-off-by: Sion Kang <siontama@gmail.com>
Signed-off-by: Sion Kang <siontama@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @agilgur5 to take a look

@agilgur5 agilgur5 self-requested a review November 20, 2023 20:12
@agilgur5 agilgur5 self-assigned this Nov 20, 2023
@agilgur5 agilgur5 added area/ui area/build Build or GithubAction/CI issues labels Nov 20, 2023
@Yaminyam
Copy link
Contributor Author

@agilgur5 What I just realized is that with the current method, the DCO check is expected to fail because the bot commit is not signed.

@Yaminyam
Copy link
Contributor Author

I think the solution would be to create a bot for signing or create a commit by the maintainer of the repo.

@agilgur5
Copy link
Member

However, there are cases where the yarn-deduplicate operation is not reflected and cannot be merged automatically.

Yea I mentioned this in #11728 (comment) and a previous Contributor Meeting (or two).
I thought updating to newer Yarn might fix this but this seems to still be a problem in newer Yarn versions too: yarnpkg/berry#4976.

There's also this Dependabot issue on the topic: dependabot/dependabot-core#5830

For dependabot pr, run yarn --cwd ui deduplicate and commit and push the changes.

Thanks for the contribution! I was thinking of adding something similar after seeing comments in the Dependabot issue pointing to foxglove/studio#4736 (current GHA workflow)

@Yaminyam
Copy link
Contributor Author

@agilgur5 It's great to hear that this was an issue that was already being addressed.
In the case of the attached pr, it operates as an independently executed workflow.
However, I added logic to the existing workflow to make execution simpler.

And it would be great if you could give us your opinion on how to solve the sign-off problem!

.github/workflows/ci-build.yaml Outdated Show resolved Hide resolved
.github/workflows/ci-build.yaml Outdated Show resolved Hide resolved
.github/workflows/ci-build.yaml Outdated Show resolved Hide resolved
@@ -401,7 +401,15 @@ jobs:
- run: yarn --cwd ui test
- run: yarn --cwd ui lint
- run: yarn --cwd ui deduplicate
- run: git diff --exit-code
- if: github.actor == 'dependabot[bot]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of adding this to the dependabot-reviewer Workflow instead, so all dependabot things are in one place. That may be slightly more complicated though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block should also have a comment describing it and why it exists

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for this job to work in dependabot-reviewer, I think it is necessary to run change-files like ci-build workflow to determine whether the ui job is running.
But I think that creates too much complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm not so much changed-files as we'd need to run parts of the ui: job itself in order to install Node, NPM, and some deps. It would indeed be simpler to leave here 🤔

Yaminyam and others added 4 commits November 21, 2023 06:26
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Sion Kang <siontama@gmail.com>
Signed-off-by: Sion Kang <siontama@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Sion Kang <siontama@gmail.com>
Signed-off-by: Sion Kang <siontama@gmail.com>
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on this one, I had some family medical emergencies pop up in late November that got me really busy.

I just have one tiny change, otherwise LGTM

.github/workflows/ci-build.yaml Outdated Show resolved Hide resolved
Yaminyam and others added 2 commits January 18, 2024 13:06
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Sion Kang <siontama@gmail.com>
@terrytangyuan terrytangyuan merged commit 568941e into argoproj:main Jan 18, 2024
27 checks passed
@Yaminyam Yaminyam deleted the ci/dependabot-auto-fix-yarn branch January 18, 2024 10:25
git config --global user.email 'github-actions[bot]@users.noreply.github.com'
git add ui/yarn.lock
git commit -s -m 'chore: deduplicate yarn.lock'
git push
Copy link
Member

@agilgur5 agilgur5 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp, now the git commit passes with an --allow-empty (per above comment), but the git push line is failing. From failing CI in #12891:

Run git config --global user.name 'github-actions[bot]'
  git config --global user.name 'github-actions[bot]'
  git config --global user.email 'github-actions[bot]@users.noreply.github.com'
  git add ui/yarn.lock
  git commit -s --allow-empty -m 'chore: deduplicate yarn.lock'
  git push
  shell: /usr/bin/bash -e {0}
  env:
    NODE_OPTIONS: --max-old-space-size=4096
[detached HEAD 4ab78f5] chore: deduplicate yarn.lock
fatal: You are not currently on a branch.
To push the history leading to the current (detached HEAD)
state now, use

    git push origin HEAD:<name-of-remote-branch>

Error: Process completed with exit code 1[2](https://github.com/argoproj/argo-workflows/actions/runs/8561962725/job/23464600770?pr=12880#step:9:2)8.

It's on a detached HEAD and therefore not on a branch that it can push. I believe this is because actions/checkout checks out the exact commit, and not a branch (which makes sense, it's safer to do that)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use $GITHUB_HEAD_REF as the branch name (per the docs, it is set for PRs, and dependabot is always a PR), so something like:

Suggested change
git push
git push origin HEAD:$GITHUB_HEAD_REF

but honestly this needs some more testing given all the errors so far. I would suggest trying this on a PR on a fork and skipping the dependabot actor check for testing purposes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm testing this in agilgur5#3. I had to add permissions.contents: write as well (which isn't great, but necessary, with no real workarounds for it).

It's still failing though, because it thinks it has something to fetch from the remote branch, even though it doesn't. So it needs some more work.

Tbh, this is getting complicated enough that it perhaps should be split out into its own independent action

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still failing though, because it thinks it has something to fetch from the remote branch, even though it doesn't.

I fixed this by adding fetch-depth to actions/checkout. It passes now.... but the PR won't merge because the new commit doesn't run any checks, since GH won't run actions on changes from other actions (to avoid an infinite loop), per previous comment.

So this whole approach isn't workable as checks will never run/pass on the final commit, meaning it will never be mergeable due to branch protections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted in #12892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants