-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
kube: honor --build=false
if specified.
#13286
kube: honor --build=false
if specified.
#13286
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
acb9ee7
to
e072787
Compare
I think the problem with this is that the image is build every time. The current behaviour is build when image not exists now it will build every time. I don't think this is a good user experience. |
pkg/domain/infra/abi/play.go
Outdated
@@ -486,7 +486,7 @@ func (ic *ContainerEngine) getImageAndLabelInfo(ctx context.Context, cwd string, | |||
if err != nil { | |||
return nil, nil, err | |||
} | |||
if (len(buildFile) > 0 && !existsLocally) || (len(buildFile) > 0 && options.Build) { | |||
if options.Build && ((len(buildFile) > 0 && !existsLocally) || (len(buildFile) > 0)) { |
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.
The (len(buildFile) > 0 && !existsLocally)
is unnecessary because you also check for only (len(buildFile) > 0)
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.
Fixed.
I think the problem with this is that the image is build every time. The current behaviour is build when image not exists now it will build every time. I don't think this is a good user experience.
@Luap99 For this i think previous behavior was wrong, we should never try to build
if --build
is not set. I am fine to move back default value of --build=false
but it changes behavior. Hence to be as close as to the previous behviour i am setting default value of build to true
.
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.
But this changes behaviour, if I have a bigger Containerfile and it start building every time this will be super annoying.
IMO it should be:
--build
or --build=true
means always build.
when --build
is not set it means only build when image not found locally
--build=false
should mean never build
You can check if a flag is set on the cli with flags.Changed("build")
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.
@Luap99 I am still confused why do we want to build if user does not specifies --build
but sure I agree lets keep the old behavior intact. I think recent commit should addresses this.
a5b9c4b
to
763d143
Compare
@Luap99 @containers/podman-maintainers PTAL |
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.
Can you clarify the build behaviour in the man page.
otherwise LGTM, @baude PTAL
`podman play kube` tries to build images even if `--build` is set to false so lets honor that and make `--build` , `true` by default so it matches the original behviour. Signed-off-by: Aditya R <arajan@redhat.com>
763d143
to
9ce61e3
Compare
--build=false
and make --build=true
by default--build=false
if specified.
/lgtm |
/hold cancel |
podman play kube
tries to build images even if--build
is set tofalse so lets honor that and make
--build
,true
by default so itmatches the original behavior.
Closes: #13285