-
Notifications
You must be signed in to change notification settings - Fork 170
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
Adding logging statements for pullsecret controller #3613
Adding logging statements for pullsecret controller #3613
Conversation
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.
nice work! I'll review again after unit tests are updated.
As discussed, we should just need to add |
@microsoft-github-policy-service agree [company="Red Hat"] |
@microsoft-github-policy-service agree company="Red Hat" |
@@ -184,15 +184,22 @@ func (r *Reconciler) ensureGlobalPullSecret(ctx context.Context, operatorSecret, | |||
// allows for simpler logic flow, when delete and create are not handled separately | |||
// this call happens only when there is a need to change, it has no significant impact on performance | |||
err := r.client.Delete(ctx, secret) | |||
r.log.Info("Global Pull secret Not Found, Creating Again") |
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.
It should be one line above or after if err
clause.
It would be better that there is always a if err
clause just after it gets err
.
err = r.client.Create(ctx, secret) | ||
if err == nil { | ||
r.log.Info("Global Pull secret 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 might be a matter of preference, but I don't want to have if clause just for one line log.
If you want to check if it failed then you can check outside of the function by checking err returned.
err = r.client.Create(ctx, secret) | |
if err == nil { | |
r.log.Info("Global Pull secret Created") | |
} | |
r.log.Info("Creating global pull secret") | |
err = r.client.Create(ctx, secret) |
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 like it the way it is personally, because it only logs when something was successful. If there's a failure, you'd get the error updating the k8s object returned instead. These logs were primarily what we were missing before, but either way works for me.
err = r.client.Update(ctx, secret) | ||
if err == nil { | ||
r.log.Info("Updated Existing Global Pull secret") | ||
} |
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.
same as above
err = r.client.Create(ctx, secret) | ||
if err == nil { | ||
r.log.Info("Global Pull secret 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.
I like it the way it is personally, because it only logs when something was successful. If there's a failure, you'd get the error updating the k8s object returned instead. These logs were primarily what we were missing before, but either way works for me.
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
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
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
FYI @slawande2 and others watching this PR - it seems to me that the previous E2E run failure was related to an orphaned E2E cluster from a cancelled agent VM. So I deleted the orphaned resource group and started a new E2E run. |
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-6600
What this PR does / why we need it:
This PR adds logging statements for the ARO Operator pullsecret controller to understand what it's updating and when, without surfacing any explicit secret data.
Test plan for issue-
How to test
First, create an OpenShift cluster and run the ARO Operator locally.
To test recreation of the pullsecret- In a seperate terminal, backup the pullsecret
oc get secret/pull-secret -n openshift-config > backup.yaml
and delete itoc delete secret/pull-secret -n openshift-config
. "Global pull secret deleted, creating again" and "Global pull secret created" will be logged.To test updating of the pullsecret-
oc get secrets pull-secret -n openshift-config -o template='{{index .data ".dockerconfigjson"}}' | base64 -d > pull-secret.json
oc set data secret/pull-secret -n openshift-config --from-file=.dockerconfigjson=./pull-secret.json
. "Updating Existing Global pull secret will be logged".