-
Notifications
You must be signed in to change notification settings - Fork 43
chore: make sure directives are correct #235
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
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.
I verified the behaviour is correct 👍
Do we need to modify these directives if we are not pushing the image?
require.Equal(t, configFile.Config.User, "root", "value does not match any of the possible root user values.") | ||
require.Equal(t, configFile.Config.WorkingDir, "/", "expected image to have root working directory") | ||
require.Equal(t, configFile.Config.Entrypoint, []string{"/.envbuilder/bin/envbuilder"}, "expected image to have envbuilder entrypoint") |
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.
probably a good idea to assert the opposite in some other test when we don't push
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.
If they are not pushed, what reference can we use to get the local image and inspect it?
@@ -342,6 +342,14 @@ func Run(ctx context.Context, options Options) error { | |||
} | |||
} | |||
|
|||
if options.PushImage { | |||
// Make sure the Dockerfile is using the correct directives to run envbuilder | |||
buildParams.DockerfileContent = buildParams.DockerfileContent + "\n" + |
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 guess that you'll have a merge conflict with the other PR: #216
This is blocked by #216 because it needs to have the embed binary to set the entrypoint correctly |
closing in favour of #246 |
Closes #217