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/workflow/rust.yml] Add docker credentials to avoid rate limit #1091

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaronkaikov
Copy link

To avoid errors such

Error toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

we should add a login to our Scylladb docker hub

Adding it

Copy link

github-actions bot commented Oct 14, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 8d9993b

@muzarski muzarski requested a review from Lorak-mmk October 15, 2024 11:42
@Lorak-mmk
Copy link
Collaborator

Thanks!

Could you update other jobs too? Those also use docker:

authenticate_test.yml
book.yml
cassandra.yml
tls.yml

@wprzytula wprzytula added the symptom/ci stability Related to continuous integration label Oct 15, 2024
@yaronkaikov
Copy link
Author

Thanks!

Could you update other jobs too? Those also use docker:

authenticate_test.yml
book.yml
cassandra.yml
tls.yml

Added

Comment on lines 27 to 34
# A separate step for building to separate measuring time of compilation and testing
- name: Update rust toolchain
run: rustup update
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
- name: Build the project
run: cargo build --verbose --tests --features "full-serialization"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you put those env on wrong step, no? The step above ("Setup 3-node Cassandra cluster") uses docker.

Comment on lines 32 to 36
- name: Run tests
run: RUST_LOG=trace cargo test --verbose authenticate_superuser -- custom_authentication --ignored
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This step does not execute docker - instead there is services section in job definition. Probably those credentials need to be there, right? However I'm not sure how to do it. It looks like there is no env section there. according to https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idservicesservice_idcredentials you can use credentials section for that, but it uses password, and I see that you are using token - so I'm not sure how to best solve it, I'm not a docker / Ghitub Actions expert :/

Comment on lines 29 to 37
- uses: actions/checkout@v3
- name: Update rust toolchain
run: rustup update
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
- name: Install mdbook
run: cargo install mdbook --no-default-features
- name: Build the project
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto about services section

Comment on lines 34 to 42
- uses: actions/checkout@v3
- name: Update rust toolchain
run: rustup update
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
- name: Check
run: cargo check --verbose --features "ssl"
working-directory: ${{env.working-directory}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto about services section.

@yaronkaikov yaronkaikov force-pushed the add-dockerhub-cred branch 2 times, most recently from fd20963 to ef4f21a Compare October 20, 2024 05:38
@yaronkaikov
Copy link
Author

@Lorak-mmk Address all comments , please review again

Lorak-mmk
Lorak-mmk previously approved these changes Oct 21, 2024
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Looks fine now. Question: does env key in services describe variables passed to docker command? Description of the key is: Sets a map of environment variables in the service container. - so I thought those are variables set in the container.

Comment on lines 29 to 36
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

docker compose won't pick these credentials up, you need to login first:

        name: Login to Docker Hub
        uses: docker/login-action@v3
        with:
          username: ${{ secrets.DOCKERHUB_USERNAME }}
          password: ${{ secrets.DOCKERHUB_TOKEN }}

Comment on lines 28 to 30
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
credentials:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will token work as password?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines 28 to 30
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
credentials:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

Comment on lines 27 to 29
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines 33 to 34
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}
```suggestion
credentials:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

@yaronkaikov yaronkaikov force-pushed the add-dockerhub-cred branch 3 times, most recently from a8961b7 to 319a7f4 Compare October 30, 2024 19:16
@yaronkaikov yaronkaikov marked this pull request as draft October 30, 2024 19:17
@yaronkaikov yaronkaikov marked this pull request as ready for review November 6, 2024 11:13
@yaronkaikov yaronkaikov force-pushed the add-dockerhub-cred branch 3 times, most recently from 145e14c to 30ff11c Compare November 6, 2024 11:58
To avoid errors such
```
Error toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
```

we should add login to our Scylladb docker hub

Adding it
@Lorak-mmk Lorak-mmk self-requested a review December 16, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
symptom/ci stability Related to continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants