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

.github: add dependabot integration #1414

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

jelly
Copy link
Member

@jelly jelly commented Sep 20, 2023

Dependabot creates a PR for us so we have to introduce a new action which updates node-modules for us.

@jelly jelly added the no-test label Sep 20, 2023
@jelly
Copy link
Member Author

jelly commented Sep 20, 2023

@martinpitt any idea how I could test this on my own fork. The obvious issue is that I have no node-modules cache repository but that sounds easily fixable. The other issues might be more difficult.

Test run here https://github.com/jelly/cockpit-podman/actions/runs/6251167467/job/16971695941?pr=8

So I should setup a NODE_CACHE repo and

secrets.NODE_CACHE_DEPLOY_KEY

This should be set

@martinpitt
Copy link
Member

@jelly I think the easier way to test this is to do this PR from an origin branch, and tell dependabot to create PRs against that.

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! Looking forward to getting this for our git-tracked node_modules/ as well!

.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
@jelly
Copy link
Member Author

jelly commented Sep 20, 2023

@jelly I think the easier way to test this is to do this PR from an origin branch, and tell dependabot to create PRs against that.

Hmm, actually it wasn't too hard to set up the node-cache but it seems the secret isn't loaded? Let's figure this out tomorrow we should have this documented somewhere as we did that for anaconda?

@jelly
Copy link
Member Author

jelly commented Sep 21, 2023

How Cockpit org loads the secrets https://github.com/cockpit-project/bots/blob/main/setup-deploy-keys

@jelly
Copy link
Member Author

jelly commented Sep 21, 2023

@jelly I think the easier way to test this is to do this PR from an origin branch, and tell dependabot to create PRs against that.

I can make a PR against origin, but no idea how to tell dependabot to create a PR against that?

Figured that out in #1416 with a WIP commit.

@jelly
Copy link
Member Author

jelly commented Sep 21, 2023

@jelly I think the easier way to test this is to do this PR from an origin branch, and tell dependabot to create PRs against that.

I can make a PR against origin, but no idea how to tell dependabot to create a PR against that?

Figured that out in #1416 with a WIP commit.

Nevermind, this won't work because of GH restrictions. This whole approach needs more work: https://stackoverflow.com/a/58740879

Basically we need to do it the reposchutz way.

It sort of works! https://github.com/jelly/cockpit-podman/actions/runs/6263734712/job/17008945862

Needs more hacking :)

@martinpitt
Copy link
Member

You can tell dependabot to use a different target branch, not sure if it helps: https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#target-branch

@jelly
Copy link
Member Author

jelly commented Sep 22, 2023

@martinpitt @allisonkarlitskaya after some 🤦 I got it working, but this requires some thought:

Successful: https://github.com/jelly/cockpit-podman/actions/runs/6274863130/job/17041081841?pr=23
Updated PR: jelly#23

So this still makes schutzbot unhappy. Do we want to set a github-changes label when we amended the commit?
Or do we not run schutzbot on these PR's so if author == dependabot?

Another issue is that we might not be able to rebase these PR's now, we can as dependabot to re-recreate the PR but I am not sure how awesome that workflow is.

Something else to think about is, should this dependabot set a no-test label to prevent the tests from being triggered when the PR is made and this workflow then can remove that label?

P.S.: @allisonkarlitskaya as author of schutzbot you also get a say here :)

@martinpitt
Copy link
Member

Your test pr failed with

fatal: remote error: upload-pack: not our ref dc4ea5cd02749a98911984a310a5ea4c6476503e

Which smells like the workflow didn't actually push the (correct or at all) updated node_modules?

Another issue is that we might not be able to rebase these PR's now

That ought to work fine? on: pull_request workflows run on force-pushes as well. What do you think would go wrong?

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! There's still some current threads from my earlier review, I closed the resolved ones.

.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
@jelly
Copy link
Member Author

jelly commented Sep 26, 2023

FYI: we might want to push as dependabot:

Looks like this PR has been edited by someone other than Dependabot. That means Dependabot can't rebase it - sorry!
If you're happy for Dependabot to recreate it from scratch, overwriting any edits, you can request @dependabot recreate.

@jelly
Copy link
Member Author

jelly commented Sep 26, 2023

An existing re-created PR: jelly#22
A fresh new Pr: jelly#25

repository: ${{ github.event.pull_request.head.repo.full_name }}
fetch-depth: 0

- name: Clear .github-changes label
Copy link
Member Author

Choose a reason for hiding this comment

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

fixme, node_modules cleared

@jelly jelly added the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Sep 27, 2023
@jelly jelly marked this pull request as ready for review September 27, 2023 14:04
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! I feel the workflow is complex and subtle enough that it could do with a high-level comment at the top what it does, especially with the juggling of the two labels.

.github/workflows/dependabot.yml Show resolved Hide resolved
.github/workflows/dependabot.yml Show resolved Hide resolved
.github/workflows/dependabot.yml Outdated Show resolved Hide resolved
Dependabot creates a PR for us so we have to introduce a new action
which updates node-modules for us.
@jelly jelly requested a review from martinpitt September 27, 2023 16:09
@github-actions github-actions bot removed the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Sep 27, 2023
@martinpitt martinpitt added the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Sep 27, 2023
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! I previously thought it was tied tightly to dependabot, but it really isn't -- in principle this works for any PR where we set the node_modules label. This is clear enough now -- I'd say, let it lose and we'll see what happens 😁

Not sure if @allisonkarlitskaya still wants to review this, though. Although there's nothing there that we can't change later on.

@jelly
Copy link
Member Author

jelly commented Sep 27, 2023

Thanks! I previously thought it was tied tightly to dependabot, but it really isn't -- in principle this works for any PR where we set the node_modules label. This is clear enough now -- I'd say, let it lose and we'll see what happens 😁

Not sure if @allisonkarlitskaya still wants to review this, though. Although there's nothing there that we can't change later on.

Oh right, I forgot to explain how it works. I can do that in a follow up.

Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Very very quick look over it and I'm happy with the approach, thanks!

@allisonkarlitskaya allisonkarlitskaya merged commit c8a723b into cockpit-project:main Sep 28, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows no-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants