chore: remove duplicated logic by function call #1080
Conversation
Signed-off-by: Wen Zhou <[email protected]>
zdtsw
requested review from
ykaliuta and
VaishnaviHire
and removed request for
etirelli and
Sara4994
|
So, what do you think about #1077 (comment) or I'm missing something? |
i did not see that one. i will give it a think tomorrow. |
Signed-off-by: Wen Zhou <[email protected]>
pkg/cluster/cert.go
Outdated
| func generateCertSecret(ctx context.Context, c client.Client, certSecret *corev1.Secret, secretName, namespace string) error { | ||
| existingSecret := &corev1.Secret{} | ||
| err := c.Get(ctx, client.ObjectKey{Name: secretName, Namespace: namespace}, existingSecret) | ||
| if err != nil { |
There was a problem hiding this comment.
I find this chain of if if if else elseif if a bit more confusing than the proposal, but fine for me if it's better for you, clear enough.
There was a problem hiding this comment.
My way of thinking behind using switch is that we have 3 cases for Get: existing object, non-existing object, error of fetching. Not 2 as for if-else.
There was a problem hiding this comment.
i was not very fond of using switch in this case, thats why tried to avoid it.
but after reading Luca's comments, maybe switch is much better for a quick return.
let me do some updates again
pkg/cluster/cert.go
Outdated
| return errors.New("failed to recreate secret with type corrected") | ||
| } | ||
| } | ||
| if isSecretOutdated(existingSecret.Data, certSecret.Data) { |
There was a problem hiding this comment.
Yeah, in the original code it was part of only one function, just kept the functionality as is, had a thought that it should be in both :) Thanks!
There was a problem hiding this comment.
i had the same thoughts, why at first we did not have outdated secret updated, unless some reason behind it.
Just wanna @VaishnaviHire take a look in case it should not be.
pkg/cluster/cert.go
Outdated
| return errors.New("failed to recreate secret with type corrected") | ||
| } | ||
| } | ||
| if isSecretOutdated(existingSecret.Data, certSecret.Data) { |
There was a problem hiding this comment.
If I properly understood the code, the isSecretOutdated function is invoked even if the secret is created or recreated but since the existingSecret is never updated (and there is no reason to do it), then it could lead to a spurious Update so the two if/else should probably return early.
There was a problem hiding this comment.
you are right.
- in the successful Create() case, it should return immediately
- in the recreateSecret() case, it does not need to check isSecretOutdated() as well
- change if-else-if to switch - fast return in recreateSecret case Signed-off-by: Wen Zhou <[email protected]>
|
Fine for me. I'll wait for @lburgazzoli final word to avoid automatic merging. |
put on a "hold" label, we are safe now 😢 + 😆 |
|
@lburgazzoli: changing LGTM is restricted to collaborators In response to this: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lburgazzoli, ykaliuta The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* chore: remove duplicated logic by function call - change if-else-if to switch - fast return in recreateSecret case --------- Signed-off-by: Wen Zhou <[email protected]>
* chore: remove duplicated logic by function call - change if-else-if to switch - fast return in recreateSecret case --------- Signed-off-by: Wen Zhou <[email protected]>
* chore: remove duplicated logic by function call - change if-else-if to switch - fast return in recreateSecret case --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit f3e6a13)
* chore: remove duplicated logic by function call - change if-else-if to switch - fast return in recreateSecret case --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit f3e6a13)
Description
ref: https://issues.redhat.com/browse/RHOAIENG-8995
How Has This Been Tested?
Screenshot or short clip
Merge criteria