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

fixes for integration test rate limit issues #8538

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Oct 16, 2022

closes #8535

Right. A few things going on here:

First 2 commits should be pretty clear. Just trying to run fewer pointless builds really. Regardless of if the builds are consuming rate limit or not, this is good. Less noise, faster builds, etc

The crucial commit is actually the third one though. What I've done is set a Personal Access Token as both a repo secret and a dependabot secret. Then in the build, we use the PAT if it is available, or fall back to the standard action token (which has the lower rate limit of 1000/hour) otherwise. This will mean PRs submitted from forks can use the standard token without us having to expose a PAT while pushes to master, dependabot PRs, etc can use the PAT with the higher rate limit. That should stop dependabot/repo ranger from completely smashing our rate limit.

@shields-ci
Copy link

shields-ci commented Oct 16, 2022

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against d7bc58d

@chris48s chris48s changed the title WIP fixes for integration test rate limit issues fixes for integration test rate limit issues Oct 16, 2022
@chris48s chris48s added the developer-experience Dev tooling, test framework, and CI label Oct 16, 2022
@chris48s chris48s added the blocker PRs and epics which block other work label Oct 20, 2022
@chris48s
Copy link
Member Author

@calebcartwright - if you get a chance, it would be really useful to get a 👍 on this one before the dependabot PRs come in for the week. In the current configuration we can only merge about 2 or 3 PRs per hour, so merging this one would be super high impact.

@calebcartwright
Copy link
Member

Actually have a question on f0ad7b6 relative to disabling the trigger for pushes to dependabot branches (assuming IUC). What's the potential impact of this in cases where we need dependabot to rebase/recreate, as those involve force pushing to the branch associated with the already created PR, and then the more rare cases where we have to manually push some commits to a dependabot branch ourselves?

@chris48s
Copy link
Member Author

As I understand it, a dependabot rebase or recreate should trigger the unhelpfully named pull_request: synchronize event

synchronize: Triggered when a pull request's head branch is updated. For example, when the head branch is updated from the base branch, when new commits are pushed to the head branch, or when the base branch is changed.

but it is possible this might need another round of tweaks. If we find there are some pull request events which are not triggering a rebuild, we could try inverting the logic: disable workflows for pull requests where the base branch is dependabot/** and enable them for pushes.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Sounds reasonable, let's give it a whirl

@chris48s chris48s merged commit 934f244 into badges:master Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker PRs and epics which block other work developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failing integration tests
3 participants