-
Notifications
You must be signed in to change notification settings - Fork 127
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
Distroless images #138
Distroless images #138
Conversation
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.
Thanks for all your effort. I found a couple of places, where we might be able to change things. What do you think?
auth/Dockerfile.nginx
Outdated
COPY nginx.conf /etc/nginx/conf.d/default.conf | ||
COPY --chown=root:root nginx.conf /etc/nginx/conf.d/my-site.conf.template | ||
|
||
RUN ls -l /etc/nginx/conf.d |
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.
I would remove the ls
.
Dockerfile
Outdated
CMD /codeengine | ||
FROM gcr.io/distroless/static-debian12 | ||
COPY --from=build-env /go/bin/app / | ||
CMD ["/app"] |
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.
Based on Docker Best Practices for choosing between run cmd and entrypoint), shouldn't we use ENTRYPOINT
here?
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.
That is a very good finding! Thanks for pointing it out!
auth/Dockerfile.nginx
Outdated
RUN ls -l /etc/nginx | ||
RUN ls -l /etc/nginx/conf.d |
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.
We should remove that.
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.
Thanks for finding it ... forgot to remove it :(
satellite-connector-to-vpc-vsi/run
Outdated
@@ -539,7 +539,7 @@ if [ $? -ne 0 ]; then | |||
fi | |||
|
|||
print_msg "\nCreating a Code Engine job '$ce_job_name' that will connect to the database ..." | |||
ibmcloud ce job create --name $ce_job_name \ | |||
IBMCLOUD_TRACE=true ibmcloud ce job create --name $ce_job_name \ |
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.
Unless we have a good reason, we shouldn't do the Tracing by default.
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.
Yep, you are right. Had this in temporary to analyze an API issue. Removed it
sessions/go.mod
Outdated
@@ -1,9 +1,10 @@ | |||
module github.com/IBM/CodeEngine/sessions | |||
|
|||
go 1.16 | |||
go 1.22 |
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.
Based on mingo
, version 1.18 would be enough.
go 1.22 | |
go 1.18 |
$ mingo -v -deps=all -check -tests .
/Users/mdiester/git/CodeEngine/sessions/sessions.go:32:19: 1 ("time".Millisecond)
github.com/onsi/[email protected] declares Go version 1.13
github.com/onsi/[email protected] declares Go version 1.14
github.com/redis/go-redis/[email protected] declares Go version 1.18
Error scanning directory: go.mod declares version 1.16 but computed minimum is 1.18 [github.com/redis/go-redis/[email protected] declares Go version 1.18]
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.
Ok cool! Wasn't aware of how this can be checked. Thanks for pointing me to it. I changed it to 1.18
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
Adjusted samples:
This PR also contains a fix for the broken cecli Dockerfile
For the auth sample I adjusted the mechanism that injects the apps namespace into the nginx config. Unfortunately, I needed to switch the user that runs the new nginx base image to root, in order to resolve permission issues while the
envsubst
tried to write the/etc/nginx/conf.d/default.conf
file