fixes #1258 controller updates deployment when headless boolean is up… #102
Conversation
…dated Signed-off-by: Horiodino <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Horiodino The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @Horiodino. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| @@ -84,6 +85,19 @@ func (r *DevfileRegistryReconciler) updateDeployment(ctx context.Context, cr *re | |||
| needsUpdating = true | |||
| } | |||
| } | |||
| headlessStatus := dep.Spec.Template.Spec.Containers[0].Env | |||
There was a problem hiding this comment.
Is it possible to rename this var to be more clear that it is the array of all env vars for the container?
There was a problem hiding this comment.
Not able to get it to update when the headless is altered. Currently deploying with
apiVersion: registry.devfile.io/v1alpha1
kind: DevfileRegistry
metadata:
name: devfile-registry
spec:
telemetry:
registryName: test
k8s:
ingressDomain: testing-registry
tls:
enabled: false
headless: true
and when running kubectl apply -n devfile registry -f <path to yaml> with this file:
apiVersion: registry.devfile.io/v1alpha1
kind: DevfileRegistry
metadata:
name: devfile-registry
spec:
telemetry:
registryName: test
k8s:
ingressDomain: testing-registry
tls:
enabled: false
headless: false
It does not update. I also noticed that if you remove headless altogether then it segfaults as a memory issue, could be a one off but worth looking into.
Was this working for you/was it tested? @michael-valdron @thepetk have either of you been able to reproduce?
| headlessStatus := dep.Spec.Template.Spec.Containers[0].Env | ||
|
|
||
| for _, env := range headlessStatus { | ||
| if env.Name == "REGISTRY_HEADLESS" { |
There was a problem hiding this comment.
I have some time to check this part of the code, so I'm not sure I understand the purpose of the env var usage here. Could you elaborate?
From my point of view, I would expect us here to:
- Compare the existing headless status with the new one.
- We could fetch the existing from
dep.Spec.Template.Spec.Headless - We could create (IIRC we don't have one) a function to get the headless status from the
registryv1alpha1.DevfileRegistryresource. - In case the two of them are different assign the current value to the existing one and update the
needsUpdatingvariable totrue.
There was a problem hiding this comment.
We could create (IIRC we don't have one) a function to get the headless status from the registryv1alpha1.DevfileRegistry resource.
im comparing on the basis of deployment , i also taught about that but saw this
https://github.com/Horiodino/registry-operator/blob/headless-boolean/pkg/registry/deployment.go#L286
so i didn't added anything.
is that same thing or different ?
There was a problem hiding this comment.
That looks like it is setting the headless value but I believe @thepetk is wanting a checker function to easily grab the headless value from a CR
Description of Changes
checks if the environment variable "REGISTRY_HEADLESS" exists in headlessStatus, parses its value to a boolean, and sets needsUpdating to true if the parsed value differs from the Headless spec in the cr.Spec
Related Issue(s)
devfile/api#1258
Acceptance Criteria
Tests
Documentation
Tests Performed
Explain what tests you personally ran to ensure the changes are functioning as expected.
How To Test
Instructions for the reviewer on how to test your changes.
Running Unit Tests
Running Integration Tests
Notes To Reviewer
Any notes you would like to include for the reviewer.