OPERATOR-240 Unable to set image urls when using operator in air gapped environment#319
Conversation
d08bf2d to
79fbee5
Compare
Codecov Report
@@ Coverage Diff @@
## master #319 +/- ##
==========================================
- Coverage 85.24% 85.23% -0.01%
==========================================
Files 44 44
Lines 9353 9361 +8
==========================================
+ Hits 7973 7979 +6
- Misses 983 984 +1
- Partials 397 398 +1
Continue to review full report at Codecov.
|
|
This PR is stale because it has been in review for 3 days with no activity. |
| require.Equal(t, "registry.io/k8s.gcr.io/pause:3.1", out) | ||
|
|
||
| out = GetImageURN("registry.io//", "k8s.gcr.io/pause:3.1") | ||
| require.Equal(t, "registry.io/pause:3.1", out) |
There was a problem hiding this comment.
Should this have been registry.io/k8s.gcr.io/pause as common registries now only has gcr.io?
There was a problem hiding this comment.
With double slashes as suffix in customRegistry (registry.io//), it always replace the rep, it does not look at common registries, this was old behavior.
There was a problem hiding this comment.
Oh right, I forgot that part.
pkg/util/util.go
Outdated
| ) | ||
|
|
||
| // UpdateCommonRegistries add additional registries to common registries. | ||
| func UpdateCommonRegistries(additionalRegistries []string) { |
There was a problem hiding this comment.
This function will eventually forget about the original common registries and keep on adding to the list.
I think we should avoid saving such a list in utils package in a variable. It is error prone to keep such a singleton util.
I would suggest that in the GetImageURN function we can just pass the list of additional registries (or the whole cluster object and then get the annotation) and replace the common registry if present in the additional list on top of the common registries.
There was a problem hiding this comment.
Sounds good, I can make it as another parameter of GetImageURN
…r in air gapped environment
5a772ce to
bb0b1cc
Compare
|
@piyush-nimbalkar I resolved your comments. |
| require.Equal(t, "registry.io/pause:3.1", out) | ||
|
|
||
| // Update it again, now k8s.gcr.io should be deleted from common registries. | ||
| out = getImageURN("gcr.io", "registry.io", "k8s.gcr.io/pause:3.1") |
There was a problem hiding this comment.
Can we add a test for comma-separated multiple registries?
pkg/util/util.go
Outdated
| } | ||
| ) | ||
|
|
||
| // UpdateCommonRegistries add additional registries to common registries. |
There was a problem hiding this comment.
Update the comment
|
@piyush-nimbalkar I resolved your last comments and merged the PR. |
…r in air gapped environment (#319) * OPERATOR-240 [Mastercard] Unable to set image urls when using operator in air gapped environment * resolve comment
…r in air gapped environment (libopenstorage#319) * OPERATOR-240 [Mastercard] Unable to set image urls when using operator in air gapped environment * resolve comment
What this PR does / why we need it:
https://portworx.atlassian.net/browse/OPERATOR-240
Which issue(s) this PR fixes (optional)
Closes #
Special notes for your reviewer: