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

Drop dependabot in favor of automated pip-tools #2592

Merged
merged 5 commits into from
Apr 5, 2023

Conversation

richardsheridan
Copy link
Contributor

@richardsheridan richardsheridan commented Mar 5, 2023

Closes #2594.

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

Merging #2592 (74cef56) into master (9338657) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2592   +/-   ##
=======================================
  Coverage   92.45%   92.45%           
=======================================
  Files         118      118           
  Lines       16342    16342           
  Branches     3155     3155           
=======================================
  Hits        15109    15109           
  Misses       1104     1104           
  Partials      129      129           
Impacted Files Coverage Δ
trio/_core/_run.py 98.49% <0.00%> (ø)

@richardsheridan
Copy link
Contributor Author

One subtle quirk of this is that we have to manually delete branches after they've been merged.

otherwise this workflow would fail in the rare case that no dependencies update within a month
@richardsheridan
Copy link
Contributor Author

@A5rocks this primarily affects you at the moment so please review and merge when you can, as this PR seems to be the preferred option in #2594

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

I think I'm ok with this, but just to confirm: when a PR fails to automerge, if we don't merge it that day (? I can't read cron, I presume this runs once daily?) CI will remake it?

ci.sh Show resolved Hide resolved
@richardsheridan
Copy link
Contributor Author

I think I'm ok with this, but just to confirm: when a PR fails to automerge, if we don't merge it that day (? I can't read cron, I presume this runs once daily?) CI will remake it?

It's once a month (best webtool ever). The branch name is distinguished by the base commit SHA so if it so happens that the automerge fails AND no other commits are pushed over the month, then the PR branch will be overwritten by force-push. Otherwise, the bot will make a new branch and PR.

@A5rocks
Copy link
Contributor

A5rocks commented Mar 10, 2023

Once a month sounds reasonable! Alright then.

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Looks alright to me! I'd like one other person skim through this before they press merge but I think this will work nicely.

@njsmith
Copy link
Member

njsmith commented Mar 10, 2023

I am excited about getting rid of dependabot spam!

I think that we'll need to switch to using a personal access token to create the PR, because otherwise Github will refuse to run CI on the PR: peter-evans/create-pull-request#48

One subtle quirk of this is that we have to manually delete branches after they've been merged.

I just flipped this setting on, which I think should take care of it?

image

@richardsheridan
Copy link
Contributor Author

I am excited about getting rid of dependabot spam!

I think that we'll need to switch to using a personal access token to create the PR, because otherwise Github will refuse to run CI on the PR: peter-evans/create-pull-request#48

I think I did know this at some point... can you help me make that token? I think it requires admin access

One subtle quirk of this is that we have to manually delete branches after they've been merged.

I just flipped this setting on, which I think should take care of it?

Nice!

@A5rocks
Copy link
Contributor

A5rocks commented Mar 10, 2023

I am excited about getting rid of dependabot spam!

I think that we'll need to switch to using a personal access token to create the PR, because otherwise Github will refuse to run CI on the PR: peter-evans/create-pull-request#48

I think I did know this at some point... can you help me make that token? I think it requires admin access

I found this just now and it seems useful; here's a list of what we could do: https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#workarounds-to-trigger-further-workflow-runs

Choice options:

  • close/reopen the PRs ;-)
  • SSH "deploy keys" (we run the actions on push assuming the branch is on this repository, I believe?)
  • Personal Access Tokens (on a machine account?)
  • ... It seems we can just use the existing trio bot to make a token for maximum coolness? But that might leak secrets somehow which would suck.

@A5rocks
Copy link
Contributor

A5rocks commented Apr 4, 2023

Hi! Just coming back here to bump this PR again. I wouldn't mind having to close/reopen our automated PRs if necessary, if we can't decide on what to do.

@richardsheridan
Copy link
Contributor Author

Right, you can merge it today and it should work just like that. In the future, someone with admin privileges can create an appropriately scoped personal token as in #2594 (comment) and simply make a PR to change the GH_TOKEN argument in the workflow to point to it and it should become fully automatic.

@njsmith
Copy link
Member

njsmith commented Apr 4, 2023 via email

@A5rocks
Copy link
Contributor

A5rocks commented Apr 5, 2023

I haven't wrapped my head around how secrets function in GitHub Actions (yet) but that certainly sounds right. I'd suggest some sort of machine account? The account making the PRs needs no permissions at all, after all. It could always make a PR from a fork (though that would complicate things).

I'll merge this now (assuming no immediate "no"s) cause there's a fix no matter how cludgey and we can figure out how to do this at a future point.

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 this pull request may close these issues.

Dependency management
3 participants