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

change --pull to be consistent with podman #1803

Closed
wants to merge 1 commit into from

Conversation

QiWang19
Copy link
Contributor

change --pull for bud and from to be consistent with podman change containers/podman#3617

Signed-off-by: Qi Wang [email protected]

pullPolicy = imagebuildah.PullIfMissing
}
if pullType == buildahcli.PullImageAlways {
Copy link
Member

Choose a reason for hiding this comment

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

Combine these together with the next section

if pullType == buildahcli.PullImageAlways || iopts.PullAlways {
pullPolicy = imagebuildah.PullAlways
}

Copy link
Member

Choose a reason for hiding this comment

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

Please hide the pullalways code and remove it from man pages.
Also throw an error if the user says to --pull-always and --pull does not match.

@@ -21,7 +21,7 @@ type fromReply struct {
creds string
format string
name string
pull bool
pull string
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change.
If I previously had the script below, it will now break.

listed in registries.conf if a local image does not exist or the image is newer
than the one in storage. Raise an error if the image is not in any listed
registry and is not present locally.
Pull image before building ("always"|"missing"|"never") (default "missing").
Copy link
Member

Choose a reason for hiding this comment

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

This is a change. The --pull used to mean to look at the registry and pull if newer. Now that is not an option, as I understand it.
Should their be another option "newer", which should be default and pull a newer image if it exists at the registy otherwise use local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If current boolean --pull also for pulling the newer image, I need to figure out to prevent buildah from pulling new image when --pull missing the image exists locally but has a newer version in the registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does buildah --pull can pull the newer image and replace the old one in the local? I tried the master buildah didn't see the update and didn't find the code implemented this functionality.
@TomSweeneyRedHat Do you know which PR I can refer to for finding newer version image from registry?

Copy link
Member

Choose a reason for hiding this comment

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

buildah bud --pull is supposed to pull the image if it doesn't exist locally or if the version on the registry is newer there than the version stored locally. I've not tested it recently to verify that it does do that.

Where it happens is fun to find.

https://github.com/containers/buildah/blob/master/new.go#L100 is where resolveImage() is called and the check happens here: https://github.com/containers/buildah/blob/master/new.go#L140

Or at least that's the theory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, @TomSweeneyRedHat but not code there, that code is for --pull-always.
if bud --pull, buildah will set options.PullPolicy = PullIfMissing, but https://github.com/containers/buildah/blob/master/new.go#L140 is inside the if options.PullPolicy == PullAlways

@QiWang19 QiWang19 force-pushed the bud_pull branch 3 times, most recently from 58ff324 to e744e0c Compare August 21, 2019 14:04
change --pull for bud and from to be consistent with podman change https://github.com/containers/libpod/pull/3617/files

Signed-off-by: Qi Wang <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Aug 27, 2019

This is a breaking change:

$ buildah from --pull fedora
fedora-working-container-11

Currently this is legal

$ ./buildah from --pull fedora
an image name (or "scratch") must be specified
ERRO exit status 1                                

With your change, it breaks.

@QiWang19
Copy link
Contributor Author

--pull was changed to string, so buildah from --pull fedora fails because no arg.
can we cheat and make --pull work both as boolean and string? I don't know how to make it.

@rhatdan
Copy link
Member

rhatdan commented Aug 28, 2019

@baude Any ideas? How much hacking would it take to see that fedora was not a valid string and switch it to a missing option.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably 5671417) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Oct 2, 2019

@QiWang19 is this still being worked on, or has it been supplanted by @TomSweeneyRedHat PR #1873

@QiWang19
Copy link
Contributor Author

QiWang19 commented Oct 2, 2019

I'm not working on this. This PR change --pull from bool to string. This is a breaking change and I don't know how to make --pull work both as boolean and string.

@rhatdan
Copy link
Member

rhatdan commented Oct 21, 2019

Lets get #1873 in and then we can discuss this.

@rhatdan rhatdan closed this Oct 21, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants