Support -ubi suffix starting 8.12.0 and 7.17.16 #7368
Conversation
| image: MapsImage, | ||
| version: "8.11.0", | ||
| suffix: "-obi1", | ||
| want: testRegistry + "/elastic-maps-service/elastic-maps-server-ubi8-obi1:8.11.0", |
There was a problem hiding this comment.
Is that not a behaviour change compared to the old logic where we would have had elastic-maps-server-obi1:8.11.0
There was a problem hiding this comment.
This was already the behavior but no unit test reflected it.
cloud-on-k8s/pkg/controller/common/container/container.go
Lines 69 to 73 in 485d511
# in pseudo-code
if containerSuffix == "" { img = "elastic-maps-server-ubi8:8.11.0" }
if containerSuffix == "-ubi8" { img = "elastic-maps-server-ubi8:8.11.0" }
if containerSuffix == "-somethingelse" { img = "elastic-maps-server-ubi8-somethingelse:8.11.0" }I checked out main, added this testcase:
{
name: "Maps old 8 image with custom suffix",
image: MapsImage,
version: "8.11.0",
suffix: "-obi1",
want: testRegistry + "/elastic-maps-service/elastic-maps-server-ubi8-obi1:8.11.0",
},The test passes:
--- PASS: TestImageRepository/Maps_old_8_image_with_custom_suffix (0.00s)There was a problem hiding this comment.
You are right I misread the old code.
| want: testRegistry + "/elasticsearch/elasticsearch-ubi8:7.17.15", | ||
| }, | ||
| { | ||
| name: "Maps 7 image with custom repository always uses -ubi suffix", |
There was a problem hiding this comment.
Do we know that maps is going to adopt the new convention? (If not we should push for it for consistency)
There was a problem hiding this comment.
maps is going to adopt the new convention.
|
|
||
| // getUBISuffix returns the UBI suffix to use depending on the given version | ||
| func getUBISuffix(v string) string { | ||
| ver := version.MustParse(v) |
There was a problem hiding this comment.
I know the spec is supposed to be validated before we reach that part of the code, but version.MustParse panics if the version cannot be parsed, which seems like a "rough" way to handle an error 😄
There was a problem hiding this comment.
I agree.
I don't know which is the best solution between these two ImageRepository signature updates:
ImageRepository(img Image, ver version.Version)string // implies that the caller does theversion.ParseImageRepository(img Image, version string) (string, error)// does theversion.Parse
I opted for the first option.
Co-authored-by: Michael Morello <michael.morello@gmail.com> Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
| if strings.HasSuffix(string(img), containerSuffix) { | ||
| return fmt.Sprintf("%s/%s:%s", containerRegistry, image, version) | ||
| suffix := "" | ||
| UBIOnly := containerSuffix == UBISuffix |
There was a problem hiding this comment.
nit: not a strong opinion but it feels to me that UBIOnly is the boolean flag that should be set by --ubi-only.
There was a problem hiding this comment.
I renamed UBIOnly in useUBISuffix in dd2ff5a.
| image: MapsImage, | ||
| version: "8.11.0", | ||
| suffix: "-obi1", | ||
| want: testRegistry + "/elastic-maps-service/elastic-maps-server-ubi8-obi1:8.11.0", |
There was a problem hiding this comment.
You are right I misread the old code.
This commit makes the operator use the -ubi suffix starting from 8.12.0 and 7.17.16 for the maps image, and for any image when the operator is configured with --ubi-only. --------- Co-authored-by: Michael Morello <michael.morello@gmail.com> Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
This commit makes the operator use the
-ubisuffix starting from8.12.0and7.17.16for the maps image, and for any image when the operator is configured with--ubi-only.