search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
108 stars 24 forks source link

Image pull policy #213

Closed synarete closed 2 years ago

synarete commented 2 years ago

Make default ImagePullPolicy=IfNotPresent in order to speed-up pod's start-up time which in turn reduces the chance of tests-failure due to timeouts.

phlogistonjohn commented 2 years ago

In general, I like the idea. Two thoughts though: I'm not sure about the necessity for the case insensitivity. Why not just do a simple string match? Also, I think our default should be the same as the k8s default - if that's ifNotPresent, great. Otherwise, I'd update to match the k8s default. If we want to a make a change for the CI runs, we can do that via the kustomize config files as a CI step.

synarete commented 2 years ago

In general, I like the idea. Two thoughts though: I'm not sure about the necessity for the case insensitivity. Why not just do a simple string match? Also, I think our default should be the same as the k8s default - if that's ifNotPresent, great. Otherwise, I'd update to match the k8s default. If we want to a make a change for the CI runs, we can do that via the kustomize config files as a CI step.

Why not simple string match: ease of use. ifNotPresent, ifnotpresent, IfNotPresent -- all valid. But as I see that is is confusing, I can fix.

I do not know what is the default value and where is it defined for each different cluster (minikube, OpenShift, k3). The idea is to enforce optimal default for testing. That said, I hoped it would fix our CI timeout failures, but it does not. So, I would recommend we keep this one as draft for now and re-consider post v0.2 release.

phlogistonjohn commented 2 years ago

In general, I like the idea. Two thoughts though: I'm not sure about the necessity for the case insensitivity. Why not just do a simple string match? Also, I think our default should be the same as the k8s default - if that's ifNotPresent, great. Otherwise, I'd update to match the k8s default. If we want to a make a change for the CI runs, we can do that via the kustomize config files as a CI step.

Why not simple string match: ease of use. ifNotPresent, ifnotpresent, IfNotPresent -- all valid. But as I see that is is confusing, I can fix.

Got it. I'd just prefer to match k8s behavior with the name. If k8s is case sensitive we should do the same.

I do not know what is the default value and where is it defined for each different cluster (minikube, OpenShift, k3). The idea is to enforce optimal default for testing.

OK, maybe we support an additional value to "leave it blank"? Just a thought, not a requirement.

That said, I hoped it would fix our CI timeout failures, but it does not. So, I would recommend we keep this one as draft for now

Sure

and re-consider post v0.2 release.

That was already what I was thinking. ;-)

phlogistonjohn commented 2 years ago

Just a reminder that this PR is still open. If you plan on following up with it, great. If not, or not with the next 3 months, it may be good to close it. It can always be reopened.

synarete commented 2 years ago

Just a reminder that this PR is still open. If you plan on following up with it, great. If not, or not with the next 3 months, it may be good to close it. It can always be reopened.

@phlogistonjohn I rebased this PR over latest master and fixed the above comment. I think now, when we are post v0.1 it is worth merging this one into master.

phlogistonjohn commented 2 years ago

/test centos-ci/sink-clustered/mini-k8s-1.24