-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(docker): support non amd64 dockerize in setup containers #7091
Conversation
Hello @tonycsoka First of all, thank you for making a contribution to DataHub! We appreciate folks who actively try to improve the project. Second, for this PR to be accepted could you please update the PR title and description to match our guidelines? See here: https://github.com/datahub-project/datahub/blob/master/docs/CONTRIBUTING.md#commit-message-format In order to ensure your PR gets merged we would highly recommend adding a description to your PR that describes the problem you are trying to solve, how did you did it and any considerations reviewers and/or users of this feature may need to have. |
Thanks @pedro93, how's that for you. |
|
||
ENV GO111MODULE=on | ||
RUN go mod tidy | ||
RUN go install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go install github.com/jwilder/dockerize@latest
might be simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistent behaviour please take the dockerize version to a well tested version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorted on the branch, not too familiar with the suggestion from @szalai1 as I'm a go virgin, but for now it'll grab the specified tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonycsoka , you don't need to clone and run all those commands to set up and build the binary, you can just run go install packagename@version
and it will install it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonycsoka did you try Peter's suggestion with |
Not yet, there's a little issue in that v0.6.1 of dockerize used this system called glock for dependency management which I'm looking at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for taking care of the review concerns. Once CI is green I think we can merge this.
Great and have a fine weekend! |
@pedro93 Are those smoke tests a little delicate, can't see why this change would cause an issue and we have 2 different failures on separate CI runs? |
Yes our tests are a little flaky sometimes when big changes to the UI happen. CI was failing due to another issue that got fixed since. Merged your PR. Thank you for the contribution! |
…-project#7091) Co-authored-by: Pedro Silva <pedro@acryl.io>
Checklist
This fixes an issue where the
amd64
binary version ofdockerize
was being used to start themysql-setup
andelasticsearch-setup
containers, regardless of the architecture of image being being, causingdatahub docker quickstart
to fail onarm64
. Instead, this fix buildsdockerize
in a staged build, and uses the resultant binary in the final image. Tried first by downloading the gzipped Source asset from thedockerize
repo, but this did not include all needed files for a build, hence going for a git clone solution.