-
Notifications
You must be signed in to change notification settings - Fork 14
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
limitador version from env var #37
Conversation
cf9f337
to
cd8c7f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense, we could quick discuss the minor comments I've left 👍🏼
@@ -372,6 +372,9 @@ spec: | |||
- --leader-elect | |||
command: | |||
- /manager | |||
env: | |||
- name: RELATED_IMAGE_LIMITADOR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename it to something like LIMITADOR_SERVICE_IMAGE
or LIMITADOR_INSTANCE_IMAGE
or just LIMITADOR_IMAGE
... regarding the last one, we could make the distinction with LIMITADOR_OPERATOR_IMAGE
if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RELATED_IMAGE prefix is commonly in OSBS for productization purposes.
I am preparing it for the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the relatedImages
section in the bundle is automatically created because of that prefix is being used.
@@ -64,9 +64,9 @@ func LimitadorDeployment(limitador *limitadorv1alpha1.Limitador) *appsv1.Deploym | |||
replicas = int32(*limitador.Spec.Replicas) | |||
} | |||
|
|||
version := DefaultVersion | |||
image := GetLimitadorImageVersion() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you are thinking on having extra capabilities for the image and repository, but if not we could simply use FetchEnv
here like:
image := helpers.FetchEnv(RELATED_IMAGE_LIMITADOR, fmt.Sprintf("%s:%s", LimitadorRepository, DefaultVersion) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only included the logic in a method to be testable in image_test.go
what
Limitador image can be read from
RELATED_IMAGE_LIMITADOR
env var. Defaults toquay.io/3scale/limitador:latest
-Required to specify the limitador image from the CSV manifest
spec.version
will override any value fromRELATED_IMAGE_LIMITADOR
.The limitador image order of preference is:
spec.version
value (only allows tag, repo will alway bequay.io/3scale/limitador
RELATED_IMAGE_LIMITADOR
env var. Image repo and tag needs to be specified. For example:quay.io/3scale/limitador:0.5.1
quay.io/3scale/limitador:latest
Verification steps
run cluster
install CRDs
run operator locally with specific
RELATED_IMAGE_LIMITADOR
env var.Create Limitador object without
spec.version
to deploy version from env var.Check limitador's version is
quay.io/3scale/limitador:0.5.1