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

fix: bash bug in containerfile #1060

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

oddgrd
Copy link
Contributor

@oddgrd oddgrd commented Jun 28, 2023

Description of change

There was a bug where if the bash conditional evaluated to false, it would fail, I am not sure why. Changing it away from the shorthand resolved it.

How has this been tested? (if applicable)

Tested by running make shuttle-provisioner with and without PROD=true and both cases work.

@oddgrd oddgrd merged commit c081d85 into shuttle-hq:main Jun 28, 2023
@oddgrd oddgrd deleted the fix/bash-bug-in-containerfile branch June 28, 2023 10:37
@jonaro00
Copy link
Member

That's strange, it was introduced a long time ago. 6843874#diff-5fcdf9b4580789697d834d1456a22bcfaa236d668fc180cad4775afc36ed5914R23

Could my bump of the Dockerfile syntax version be the cause? It was only rc -> official.

Anyways, if the condition evaluates to false, the command exits with non zero, so that's the "reason" it fails. Not sure why it worked before tho :)

@oddgrd
Copy link
Contributor Author

oddgrd commented Jun 28, 2023

Oops, I mean the build failed if it evaluated to true. I believe it always evaluated to false before your PR since it was missing the ARG CARGO_PROFILE, so your PR fixed it but that revealed a bash bug that I don't quite understand 😅

@jonaro00
Copy link
Member

Then it could be that Dockerfile evaluates RUN lines with sh and not bash, which would explain why it fails at [[.
The same bug was fixed here https://github.com/shuttle-hq/shuttle/pull/989/files#diff-682a893302ebb3f2ed26573ec16e2f46e6642492a65a6853405963b7d989300eR1

AlphaKeks pushed a commit to AlphaKeks/shuttle that referenced this pull request Jul 21, 2023
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.

2 participants