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

support the new docker build --pull option #1191

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rohanKanojia
Copy link
Member

Extension of PR: #626

@rohanKanojia rohanKanojia force-pushed the pullReq626 branch 2 times, most recently from 1b3fda9 to b6bb1af Compare April 7, 2019 08:27
@rohanKanojia rohanKanojia requested a review from rhuss April 7, 2019 08:35
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

As mentioned in #626 i'm not positive to include support --pull (our current auto-pull mode is good enough), but we should include autoPull also on the build configuration.

@@ -81,6 +81,9 @@ A provided `<from>` takes precedence over the name given here. This tag is usefu
| *maintainer*
| The author (`MAINTAINER`) field for the generated image

| *pull*
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you please check, I think the entries are alphabetically order in the table.

@rhuss
Copy link
Collaborator

rhuss commented Aug 10, 2019

@rohanKanojia not sure what the status of this PR is ? Should we close is ? Looks like it got out-of-sync quite a bit.

@rohanKanojia
Copy link
Member Author

Let me check it on Monday, If updating it is painful, then I would close this. Otherwise I would try to modify it to accomodate that autoPull option

@rhuss
Copy link
Collaborator

rhuss commented Aug 10, 2019

@rohanKanojia no worries, I probably don't have a chance to work on dmp before my holidays. So take your time ;-)

return buildConfig.getAutoPull().equalsIgnoreCase("TRUE") ?
ImagePullPolicy.IfNotPresent.name() :
ImagePullPolicy.Never.name();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was a long time ago, but what actually why do we need both imagePullPolicy and autoPull as properties ? It looks like they are really doing the same and autoPull could always be interfered from an imagePullPolicy.

@@ -137,6 +137,7 @@ protected void buildImage(ImageConfiguration imageConfig, MojoParameters params,
// auto is now supported by docker, consider switching?
BuildOptions opts =
new BuildOptions(buildConfig.getBuildOptions())
.autoPull(buildConfig.getAutoPull())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we could put in the value calculated from image pull policy ?

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks, the PR makes totally sense, but I'm still wondering why we just can use the pullPolicy every instead of having an additional autoPull. Historically autoPull was first but IMO serve the same purpose than imagePullPolicy. So instead of introducing yet another autoPull field maybe we should just ensure that imagePullPolicy works properly on the BuildConfiguration ?

But maybe I misunderstood something and autoPull is required in addition to imagePullPolicy.

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Also, I can't see the original purpose of the PR anymore that introduce the option pull=1 to BuildOptions when possible. Looks like it had been dropped accidentally :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants