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 build-all-in-one-image script #4819

Merged

Conversation

albertteoh
Copy link
Contributor

@albertteoh albertteoh commented Oct 7, 2023

Description of the changes

+ '[main' '!=' 'pr-only]'
scripts/build-all-in-one-image.sh: line 57: [main: command not found

How was this change tested?

  • Tested using a simple bash script to reproduce the problem.

Checklist

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
@albertteoh albertteoh requested a review from a team as a code owner October 7, 2023 20:14
@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

All modified lines are covered by tests ✅

see 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

usually double [[]] are used for expressions

@yurishkuro
Copy link
Member

if it's missing from docker hub, the workflow should've failed, let's make sure this happens

@@ -54,7 +54,7 @@ bash scripts/build-upload-a-docker-image.sh -b -c all-in-one -d cmd/all-in-one -


#do not run debug image build when it is pr-only
if ["$mode" != "pr-only"]; then
if [ "$mode" != "pr-only" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ "$mode" != "pr-only" ]; then
if [[ "$mode" != "pr-only" ]]; then

@yurishkuro
Copy link
Member

I think the reason there is no 1.50 image is because the build did not recognize semver tag and published this as latest only:

+ docker buildx build --output 'type=image, push=true' --progress=plain --target release --build-arg base_image=localhost:5000/baseimg_alpine:latest --build-arg debug_image=localhost:5000/debugimg_alpine:latest --platform=linux/amd64,linux/s390x,linux/ppc64le,linux/arm64 --file cmd/all-in-one/Dockerfile --tag docker.io/jaegertracing/all-in-one --tag docker.io/jaegertracing/all-in-one:latest --tag quay.io/jaegertracing/all-in-one --tag quay.io/jaegertracing/all-in-one:latest --tag docker.io/jaegertracing/all-in-one-snapshot:2bb7ee46f8f8ed9310515f4e68eca32afeae4589 --tag quay.io/jaegertracing/all-in-one-snapshot:2bb7ee46f8f8ed9310515f4e68eca32afeae4589 cmd/all-in-one

@yurishkuro
Copy link
Member

So the workflow was indeed successful. My other fix should solve that, hopefully.

@yurishkuro yurishkuro merged commit 9831d87 into jaegertracing:main Oct 7, 2023
31 checks passed
@albertteoh albertteoh deleted the fix-build-all-in-one-script branch October 7, 2023 21:23
@albertteoh
Copy link
Contributor Author

if it's missing from docker hub, the workflow should've failed, let's make sure this happens

I was wondering why we didn't get a non-zero exit status on this failure even though we have set -exu.

From this SO answer:

The exit status of the if command shall be the exit status of the then or else compound-list that was executed, or zero, if none was executed.

So since the then block wasn't executed, it just defaults to zero.

I'm not sure if there's anything reasonable we can do except making sure we use the correct bash syntax.

@albertteoh
Copy link
Contributor Author

I'm not sure if there's anything reasonable we can do except making sure we use the correct bash syntax.

Perhaps we could do something to prevent this sort of error like using https://github.com/koalaman/shellcheck (github action available: https://github.com/marketplace/actions/shellcheck).

What do you think?

@yurishkuro
Copy link
Member

great idea -- #4822

@yurishkuro yurishkuro added the changelog:ci Change related to continuous integration / testing label Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:ci Change related to continuous integration / testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants