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

Allow configuring pull policy (with Always by default for nightlies) #57

Merged
merged 4 commits into from
Jul 26, 2019

Conversation

davidfestal
Copy link
Contributor

Currently, the pull policy for the Che, Keycloak and Postgres containers is fixed and set at IfNotPresent.
Obviously this make the use of the nightly operator (and nightly Che components) useless.

This PR:

  • adds fields in the CheCluster custom resource to drive the image pull policy of the various Che components (Che, Keycloak, etc...)
  • If those fields are not set (or set to empty string) then a default pull policy is applied according to the following rule:
    • use Always if the related docker image tag is latest or nightly
    • use IfNotPresent in other cases

Please consider this PR as a followup of issue eclipse-che/che#13780 that was part of endgame plan.

Signed-off-by: David Festal <dfestal@redhat.com>
in Che, Keycloak and Postgres deployments

Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

seems legit.

@@ -143,6 +147,8 @@ type CheClusterSpecAuth struct {
OauthSecret string `json:"oAuthSecret"`
// KeycloakImage is image:tag used in Keycloak deployment
KeycloakImage string `json:"identityProviderImage"`
// KeycloakImagePullPolicy is the image pull policy used in Keycloak registry deployment: default value is Always
KeycloakImagePullPolicy corev1.PullPolicy `json:"identityProviderImagePullPolicy"`
Copy link
Contributor

Choose a reason for hiding this comment

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

it's too bad we have Keycloak* varibles mixed in with identityProvider* images but that's a refactoring for another time :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the problem here is that it would be a breaking change in the API. So we would have to manage several versions of the API, which is not straightforward afaik.

@@ -57,3 +59,15 @@ const (
DefaultSecurityContextFsGroup = "1724"
DefaultSecurityContextRunAsUser = "1724"
)

func DefaultPullPolicyFromDockerImage(dockerImage string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised about that @nickboldt didn't comment about your usage of the docker word here :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha. He still can do a PR afterwards if he prefers other words :-)

@nickboldt
Copy link
Contributor

Once this is merged, I expect to see Always as the pull policy here:

image

(Screenshot taken after doing a chectl operator deployment of che 7 nightly, with custom images. See https://github.com/redhat-developer/codeready-workspaces/blob/master/devdoc/building/che7-custom-resource-airgap.adoc for steps to reproduce.)

@davidfestal
Copy link
Contributor Author

Yes, exactly.

@davidfestal
Copy link
Contributor Author

If you use chectl, you can even have this today by overriding the che-operator-image to quay.io/dfestal/che-operator:add-pull-policies I assume

@nickboldt
Copy link
Contributor

So is this being merged for 7 RC5 / GA?

@davidfestal davidfestal merged commit d887a50 into master Jul 26, 2019
@sleshchenko sleshchenko deleted the add-pull-policies branch October 18, 2019 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants