-
Notifications
You must be signed in to change notification settings - Fork 627
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
feat: bump Argo CD version to 2.2.2 #532
Conversation
@iam-veeramalla Should we update the CRDs for |
Thats a good one. Let me go and check :) |
Please update. |
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
@@ -680,15 +680,17 @@ func (r *ReconcileArgoCD) reconcileSSHKnownHosts(cr *argoprojv1a1.ArgoCD) error | |||
// reconcileTLSCerts will ensure that the ArgoCD TLS Certs ConfigMap is present. | |||
func (r *ReconcileArgoCD) reconcileTLSCerts(cr *argoprojv1a1.ArgoCD) error { | |||
cm := newConfigMapWithName(common.ArgoCDTLSCertsConfigMapName, cr) | |||
cm.Data = getInitialTLSCerts(cr) |
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.
where are the changes in configmap.go coming from?
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.
There are changes to reconciler logic. What amount of testing does it come with?
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 am equally surprised. I dont know why this unit test was failing.
The issue is coming from isObjectFound method but I dont see any changes made.
return !apierrors.IsNotFound(FetchObject(client, namespace, name, obj)) |
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.
To be honest there is no change in the logic. It's mostly refactoring. So according to me no additional testing is 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.
Which unit test was failing?
The change of logic/behavior is that now it only sets controller owner reference when object does not exist (or when a new cm is created).
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.
This is the failing unit test
TestReconcileArgoCD_reconcileTLSCerts_withUpdate
However, I figured out the root cause of the issue.
The reconciliation logic for reconcileTLSCerts
currently works like this.
- creates a configmap object.
- gets the tls certificates from the Argo CD CR and updates the configmap object.
- it uses
argoutil.IsObjectFound
method to identify the existence ofargocd-tls-certs-cm
configmap in the kubernetes cluster. - If the object exists, it updates the existing configmap with the tls certificates.
While this looks good,
The latest version of controller-runtime overrides the object instead of updating
.
Please refer to the below PR.
kubernetes-sigs/controller-runtime#1651
So, we need to slightly modify the steps and verify the existence of configmap first.
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.
The change of logic/behavior is that now it only sets controller owner reference when object does not exist (or when a new cm is created).
That makes sense as existing objects(configmaps) would already have the owner reference set.
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.
kubernetes-sigs/controller-runtime#1651 is a fix to the fake client. The question is that is the current reconciler not doing the right thing and needs to be changed? If the fix to the fake client only affects testing, the failing test should be fixed.
@@ -962,6 +962,11 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoprojv1a1.ArgoCD) error | |||
desiredImage := getRepoServerContainerImage(cr) | |||
if actualImage != desiredImage { | |||
existing.Spec.Template.Spec.Containers[0].Image = desiredImage | |||
if existing.Spec.Template.ObjectMeta.Labels == nil { |
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.
Is this change also made due to a failing test?
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.
Yes, It is made to fix the failing test. I will try to investigate more on what was the reason for unit test failure tomorrow and get back.
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 can only suspect it has something to do with the kubernetes client version update to 1.22.2
.
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.
However, I figured out the root cause of the issue.
The latest version of controller-runtime overrides the object instead of updating.
Please refer to the below PR.
kubernetes-sigs/controller-runtime#1651
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.
LGTM
Thanks @iam-veeramalla
Signed-off-by: iam-veeramalla abhishek.veeramalla@gmail.com
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
upgrades Argo CD to v2.2.2
Have you updated the necessary documentation?
NA
How to test changes / Special notes to the reviewer: