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

Add Docker Hub auth to rate-limited CI workflows #5373

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

daveqnet
Copy link
Contributor

@daveqnet daveqnet commented Oct 14, 2024

Closes OPS-772

Description Of Changes

  • Add Docker Hub auth to Cypress E2E Tests workflow
  • Add Docker Hub auth to Backend Code Checks workflow
  • Removes hardcoded Docker username in Publish Docker Images workflow and upgrades action to latest (v3)

Code Changes

  • Adding Docker Hub auth for a premium user will increase the rate limit to 5000/day instead of the 100/6 hours for unauth'd Docker users.

Steps to Confirm

  • Run currently failing CI jobs for modified workflows and ensure they pass.

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
    • documentation complete, PR opened in fidesdocs
    • documentation issue created in fidesdocs
    • if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!

@daveqnet daveqnet self-assigned this Oct 14, 2024
Copy link

vercel bot commented Oct 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Oct 14, 2024 6:19pm

@daveqnet daveqnet added the github_actions Pull requests that update GitHub Actions code label Oct 14, 2024
@daveqnet daveqnet marked this pull request as ready for review October 14, 2024 16:40
Copy link
Contributor

@RobertKeyser RobertKeyser left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@RobertKeyser RobertKeyser self-requested a review October 14, 2024 16:42
Copy link
Contributor

@RobertKeyser RobertKeyser left a comment

Choose a reason for hiding this comment

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

I did notice in other workflows, we define them as environment variables with the following pattern under the env key

  DOCKER_USER: ethycaci
  DOCKER_RO_TOKEN: ${{ secrets.DOCKER_RO_TOKEN }}

And then use those envs instead of the secrets directly.

I think this should work, but if it doesn't, we'll go that route. Either way, approved.

Copy link

cypress bot commented Oct 14, 2024

fides    Run #10407

Run Properties:  status check passed Passed #10407  •  git commit 697c534d94 ℹ️: Merge 21119d9a8be410f3c6abc3d834dd85446dbefb95 into 33ef06491f14f763578eec44fbac...
Project fides
Run status status check passed Passed #10407
Run duration 00m 40s
Commit git commit 697c534d94 ℹ️: Merge 21119d9a8be410f3c6abc3d834dd85446dbefb95 into 33ef06491f14f763578eec44fbac...
Committer Dave Quinlan
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

@daveqnet daveqnet added the do not merge Please don't merge yet, bad things will happen if you do label Oct 14, 2024
@daveqnet
Copy link
Contributor Author

Adding Do Not Merge label. The Backend Code Checks workflow needs to be rethought as the actions step logs into Docker Hub... and then logs out before the next job where auth is needed 🥹
Screenshot 2024-10-14 at 17 55 32

Copy link
Contributor

@RobertKeyser RobertKeyser left a comment

Choose a reason for hiding this comment

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

Good catch! Looks good to me.

@daveqnet
Copy link
Contributor Author

Thanks Rob! Assuming CI passes, I'll merge this in the morning.

@daveqnet daveqnet merged commit d30bf76 into main Oct 15, 2024
37 checks passed
@daveqnet daveqnet deleted the daveqnet/OPS-772/fix-dh-rate-limited-workflows branch October 15, 2024 10:06
@daveqnet daveqnet removed the do not merge Please don't merge yet, bad things will happen if you do label Oct 15, 2024
Copy link

cypress bot commented Oct 15, 2024

fides    Run #10417

Run Properties:  status check passed Passed #10417  •  git commit d30bf76852: Add Docker Hub auth to rate-limited CI workflows (#5373)
Project fides
Run status status check passed Passed #10417
Run duration 00m 38s
Commit git commit d30bf76852: Add Docker Hub auth to rate-limited CI workflows (#5373)
Committer Dave Quinlan
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants