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.

Sign up for GitHub

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

Jump to bottom

Use ubi instead of golang image for main Dockerbuild #548

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Use ubi instead of golang image for main Dockerbuild #548

merged 4 commits into from
Oct 25, 2023

Conversation

idlewis
Copy link
Collaborator

@idlewis idlewis commented May 30, 2023

To deal with dockerhub rate limits

See WASdev/websphere-liberty-operator#270

@idlewis idlewis marked this pull request as draft May 30, 2023 13:45
@idlewis idlewis self-assigned this Aug 30, 2023
@idlewis idlewis force-pushed the docker-base branch 2 times, most recently from 92bbd5a to 583b589 Compare October 9, 2023 12:44
@idlewis idlewis requested a review from halim-lee October 9, 2023 12:47
@idlewis idlewis marked this pull request as ready for review October 9, 2023 12:47
halim-lee
halim-lee approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@halim-lee halim-lee left a comment

Choose a reason for hiding this comment

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

Looks good!

leochr
leochr reviewed Oct 10, 2023
View reviewed changes
Copy link
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

@idlewis Wondering whether the operator binary from the new approach be compared with the current approach to ensure there isn't a significant difference

Dockerfile Outdated
@@ -1,5 +1,9 @@
# Build the manager binary
FROM golang:1.20 as builder
FROM registry.access.redhat.com/ubi8 as builder
Copy link
Member

Choose a reason for hiding this comment

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

Can registry.access.redhat.com/ubi8/ubi-minimal:latest be used instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@leochr I've pushed some updates to use ubi-minimal.
I've tested locally and have a pipeline build running now.

@idlewis
Copy link
Collaborator Author

idlewis commented Oct 11, 2023

@idlewis Wondering whether the operator binary from the new approach be compared with the current approach to ensure there isn't a significant difference

@leochr I have had a look around and I can't find a method to do a meaningful compare.
go version -m <go binary>
lists the go version and modules used to compile a binary, that is the best I have found.

One issue the above command did show is that I was using go 1.20.0 rather than 1.20.10. It seems the golang 1.20 docker image uses the 'latest' 1.20 version of go. However, the go download URL for 1.20 seems to specifically get 1.20.0. I haven't found an alternative download URL. This would mean we would have to update the docker file regularly to keep up with minor version updates.

leochr
leochr reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

@idlewis Thanks for the PR. Added a few comments.

Dockerfile Outdated
FROM golang:1.20 as builder
FROM registry.access.redhat.com/ubi8-minimal:latest as builder
ARG GO_PLATFORM=amd64
env PATH=$PATH:/usr/local/go/bin
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ENV instead of env?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. It seems to work with 'env' but that isnt' documented, so I've updated it.

Dockerfile Outdated
ARG GO_PLATFORM=amd64
env PATH=$PATH:/usr/local/go/bin
RUN microdnf install tar gzip
RUN curl -L --output - "https://golang.org/dl/go1.20.10.linux-${GO_PLATFORM}.tar.gz" | tar -xz -C /usr/local/
Copy link
Member

Choose a reason for hiding this comment

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

The Go version should be parameterized. Link it to the OnePipeline's config go-version

https://github.ibm.com/websphere/operators/blob/3f38eb6677e355cdc7a5320a0c2cb9690709301f/scripts/pipeline/containerize-stage.sh#L109

@leochr
Copy link
Member

leochr commented Oct 23, 2023

@idlewis The instructions at https://go.dev/doc/install state to use
rm -rf /usr/local/go && tar -C /usr/local -xzf go1.21.3.linux-amd64.tar.gz

It doesn't seem like UBI image includes /usr/local/go but it would be good to remove the directory.
Also wondering if the -f option should be used on tar command

@idlewis
Copy link
Collaborator Author

idlewis commented Oct 24, 2023

@idlewis The instructions at https://go.dev/doc/install state to use rm -rf /usr/local/go && tar -C /usr/local -xzf go1.21.3.linux-amd64.tar.gz

It doesn't seem like UBI image includes /usr/local/go but it would be good to remove the directory. Also wondering if the -f option should be used on tar command

I've added the rm command.
The -f isn't needed in the tar command as we are extracting from stdin, instead of a file

@idlewis idlewis force-pushed the docker-base branch 2 times, most recently from 54c4b55 to 19d48fc Compare October 25, 2023 11:25
@idlewis
Add build arg for docker version
fb7cbe2 
and fix env -> ENV

And if no version is supplied, grab a default by parsing
go.mod, in the same way that the go install script does
leochr
leochr approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

@idlewis Looks good. Thank you

@leochr leochr merged commit e24b121 into main Oct 25, 2023
@leochr leochr deleted the docker-base branch October 25, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halim-lee halim-lee halim-lee approved these changes

@leochr leochr leochr approved these changes

Assignees

@idlewis idlewis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@idlewis @leochr @halim-lee