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

Remove unnecessary installation of Rust from CI step #1288

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

erikjohnston
Copy link
Member

Follows on from #1287

@erikjohnston erikjohnston requested a review from a team as a code owner September 8, 2022 14:53
@DMRobertson
Copy link
Contributor

Err is this meant to fail CI?

(Tempted to just leave it as it is and not ask too many questions)

@erikjohnston
Copy link
Member Author

I am extremely confused as to wtf is going on now.

@squahtx
Copy link
Contributor

squahtx commented Sep 9, 2022

The docker build step where we install the rust toolchain only adds 32B to the image(????)
https://hub.docker.com/layers/matrixdotorg/sytest-synapse/focal/images/sha256-2206b265dc62720753ee1dbe43b03002f293455a905ee8e8e2a5cd0fc67c2bae?context=explore

Looks like the install command failed, but the GHA still passed.
https://github.com/matrix-org/sytest/runs/8253226528?check_suite_focus=true#step:6:957

@erikjohnston
Copy link
Member Author

Aha, that's annoying. Why on earth did the docker image build not fail :/

@erikjohnston
Copy link
Member Author

Hopefully #1290 fixes the docker image

@DMRobertson
Copy link
Contributor

Aha, that's annoying. Why on earth did the docker image build not fail :/

See https://github.com/docker/docker.github.io/blob/master/develop/develop-images/dockerfile_best-practices.md#using-pipes

https://stackoverflow.com/questions/69684133/dl4006-warning-set-the-shell-option-o-pipefail-before-run-with-a-pipe-in-it seems to suggest that a SHELL directive can be used to set this globally in the dockerfile.

https://github.com/hadolint/hadolint might be interesting to pick this up?

@erikjohnston
Copy link
Member Author

This is now fixed!

Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

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

lgtm

@erikjohnston erikjohnston merged commit bebf431 into develop Sep 12, 2022
@erikjohnston erikjohnston deleted the erikj/sytest_rust branch September 12, 2022 16:04
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.

3 participants