Fixes for the User CRDs #533
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
ribaraka
commented
Aug 23, 2023
ribaraka
commented
- removed redundant finalizers in user functionality, which is set after each new cluster assignment.
- from now on we will use only one and only finalizer that is bound between a User CRD and the Secret.
- added a const containing a message for user deletion
- cleaned up a code
- moved the adding finalizers at the beginning of the reconcile func
- fixes along the way for some user controllers.
- Closes [User CRD] New finilizer to a deleting resource #532
ribaraka
added
bug
ribaraka
requested review from
taaraora,
testisnullus,
tengu-alt,
OleksiienkoMykyta and
worryg0d
testisnullus
approved these changes
Aug 23, 2023
pkg/models/errors.go
Outdated
| @@ -54,6 +54,6 @@ var ( | |||
| ErrImmutableSecretRef = errors.New("secret reference is immutable") | |||
| ErrEmptySecretRef = errors.New("secretRef.name and secretRef.namespace should not be empty") | |||
| ErrMissingSecretKeys = errors.New("the secret is missing the correct keys for the user") | |||
| ErrUserStillExist = errors.New("the user is still attached to cluster") | |||
| ErrUserStillExist = errors.New("the user is still attached to cluster. If you want to delete the user, first remove the user from cluster specification") | |||
There was a problem hiding this comment.
user is still attached to cluster. If you want to delete the user, first remove the user from cluster specification -> the user is still attached to the cluster. If you want to delete the user, remove the user from the cluster specification first
worryg0d
reviewed
Aug 23, 2023
| func getDeletionUserFinalizer(clusterID string) string { | ||
| return models.DeletionUserFinalizer + clusterID | ||
| } | ||
| const msgDeleteUser = "If you want to delete the user, first remove the user from cluster specification" |
There was a problem hiding this comment.
Seems better to say:
Suggested change
| const msgDeleteUser = "If you want to delete the user, first remove the user from cluster specification" | |
| const msgDeleteUser = "If you want to delete the user, remove it from the clusters specifications first" |
worryg0d
reviewed
Aug 23, 2023
Comment on lines
417
to
348
| l.Error(models.ErrUserStillExist, "please remove the user from the cluster specification", | ||
| l.Error(models.ErrUserStillExist, "If you want to delete the user, first remove the user from cluster specification", | ||
| "username", username, "cluster ID", clusterID) | ||
| r.EventRecorder.Event(user, models.Warning, models.DeletingEvent, | ||
| "The user is still attached to cluster, please remove the user from the cluster specification.") | ||
| "If you want to delete the user, first remove the user from cluster specification") |
There was a problem hiding this comment.
Please use here msgDeleteUser as you do in other similar cases
tengu-alt
approved these changes
Aug 25, 2023
worryg0d
approved these changes
Aug 25, 2023
|
|
||
| return models.ReconcileRequeue, nil | ||
| } | ||
|
|
||
| if controllerutil.AddFinalizer(secret, user.GetDeletionFinalizer()) { |
There was a problem hiding this comment.
I think it would be better to add finalizer after getting creds
OleksiienkoMykyta
approved these changes
Aug 25, 2023
taaraora
approved these changes
Aug 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.