Fix bug where ArgoCD removes nodePlacement stanza from configuration #740
Conversation
|
/retest |
1 similar comment
|
/retest |
| existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement | ||
| changed = true | ||
| // Checking if default nodePlacement is nil to verify whether the patch is done using GitopService CR or ArgoCD CR | ||
| // if user is patching nodePlacement through GitopsService CR, then the above written logic for GitopsService will add custom values to defaultArgoCDInstance's nodePlacement, |
There was a problem hiding this comment.
The logic is that unless any change is made in GitopService CR, don't check the nodePlacement spec right?
That will allow users to manage the default instance via ArgoCD CR.
The comment is confusing and not sure if needed.
There was a problem hiding this comment.
In the bug, user is trying to patch nodePlacement using oc patch argocd ... and logic present is checking for nodePlacement when user tries to patch using gitopsservice oc patch gitopsservice ... . Basically in comment it is mentioned that if the user is trying to patch argocd using gitopsservice then defaultArgoCDInstance.Spec.NodePlacement will not be nil and it will update argocd(i.e, existingArgoCD.Spec.NodePlacement ) based on that, otherwise defaultArgoCDInstance.Spec.NodePlacement will be nil and it will not update the existingArgoCD.Spec.NodePlacement now as I have added If condition here to check this, I have added the explanation in the description, please let me know if its not making sense
There was a problem hiding this comment.
IIUC, what Saumeya is trying to convey is, instead of this comment i.e. explaining how this part of code works, if we can just reduce the comment to what it does.
Maybe we can just write
// if user is patching nodePlacement through GitopsService CR, then existingAgoCd NodePlacement is updated.
There was a problem hiding this comment.
got it, I will update this, thanks Saumeya and Anand
|
Also, did you verify why the users are unable to patch the resource via the GitopService CR? why does that not add the required nodeSelectors, maybe there is another bug in that. |
Yes I tried reproducing both the bugs , I reproduced one of the bug where user is trying to patch using argocd (oc patch argocd) and the second one((oc patch gitopsservice) I did not see this bug, I tried every steps to reproduce it, but not seeing this bug and I am able to patch argocd via GitopsService CR |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/retest |
|
/retest |
|
LGTM, rebasing to latest to see if that fixes the test failures |
Signed-off-by: Rizwana777 <[email protected]>
|
/retest |
|
/test v4.12-kuttl-sequential |
|
/retest |
|
CI issue? /retest |
|
Hi @jgwest, there are four sequential test failures that are recurring for this PR so I don't think re trigger would help. Retesting without proper investigation leads to more resource consumption and at the moment we are trying to minimise our cloud cost. I have suggested @Rizwana777 to take a look at the tests that are failing and run them individually with her change to debug further, maybe some tests need updates? 1-071_validate_node_selectors Sequential test history: |
Signed-off-by: Rizwana777 <[email protected]>
There was a problem hiding this comment.
LGTM, thanks @Rizwana777, and thanks everyone for your reviews/input!
…edhat-developer#740) * Fix bug where ArgoCD removes nodePlacement stanza from configuration Signed-off-by: Rizwana777 <[email protected]> * Add unit test for argocd nodeplacement Signed-off-by: Rizwana777 <[email protected]> --------- Signed-off-by: Rizwana777 <[email protected]>
What type of PR is this?
/kind bug
JIRA - https://issues.redhat.com/browse/GITOPS-4185
We were not able to patch the NodePlacement directly through ArgoCD CR, if we do, it removes the nodePlacement from ArgoCD CR while reconciling because of this https://github.com/redhat-developer/gitops-operator/blob/master/controllers/gitopsservice_controller.go#L475 here
defaultArgoCDInstance.Spec.NodePlacementis nil as it not getting updated anywhere else in code,defaultArgoCDInstance.Spec.NodePlacementis only getting updated if we update it through GitOpsService CR or else it updateexistingArgoCD.Spec.NodePlacementas nil because of the reason I have mentioned above , but if a user wants to add custom nodePlacement he/she should patch nodePlacement through GitOpsService CR and it is working as expected I see no bug in code and code is validgitops-operator/controllers/gitopsservice_controller.go
Line 392 in 01251d2
I have updated the code here -
gitops-operator/controllers/gitopsservice_controller.go
Line 475 in 01251d2
which is checking for nil , the logic mentioned in the PR states that , it will check for
defaultArgoCDInstance.Spec.NodePlacementnot equal to nil (it means nodePlacePlacement is updated through GitOps service CR and the values got updated here in if condition) else it will not enter the if condition to check ifdefaultArgoCDInstance.Spec.NodePlacementis nil (it means nodePlacePlacement is updated through ArgoCD by user and it should not replace the existingArgoCD with nil value, that is the changes made by user will not get reverted back as mentioned in the bug when it reconciles)This is what I have came up with for the bug mentioned above, please do let me know your suggestions, thank you