issue-469, delete all users from deleted cluster #481
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
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
2 times, most recently
from
2b057d8
to
2ec620f
Compare
OleksiienkoMykyta
requested review from
ribaraka,
testisnullus and
worryg0d
and removed request for
ribaraka
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
from
2ec620f
to
7c3c9eb
Compare
OleksiienkoMykyta
changed the title
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
2 times, most recently
from
f614e6b
to
6729b4f
Compare
worryg0d
approved these changes
Jul 19, 2023
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
2 times, most recently
from
58cb36a
to
8861645
Compare
ribaraka
requested changes
Jul 19, 2023
Comment on lines
217
to
223
| err = r.detachUserFromDeletedCluster(ctx, clusterID, user, l) | ||
| if err != nil { | ||
| l.Error(err, "Cannot delete Redis user resource", | ||
| "secret name", secret.Name, | ||
| "secret namespace", secret.Namespace) | ||
| r.EventRecorder.Eventf(user, models.Warning, models.PatchFailed, | ||
| "Resource deleting is failed. Reason: %v", err) |
There was a problem hiding this comment.
- it's not a patch operation
- typos in the logs. deleting -> detach
| return err | ||
| } | ||
|
|
||
| patch = user.NewPatch() |
There was a problem hiding this comment.
I don't think we need to reinitialize a patch
| for _, ref := range redis.Spec.UserRefs { | ||
| err = r.detachUserResource(ctx, logger, redis, ref) | ||
| if err != nil { | ||
| logger.Error(err, "Cannot delete Redis user cluster", |
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
from
8861645
to
3dd3402
Compare
testisnullus
requested changes
Jul 20, 2023
| "secret name", secret.Name, | ||
| "secret namespace", secret.Namespace) | ||
| r.EventRecorder.Eventf(user, models.Warning, models.UpdatedEvent, | ||
| "Cannot assign Cassandra user to a k8s secret. Reason: %v", err) | ||
| "Cannot assign Redis user to a k8s secret. Reason: %v", err) |
There was a problem hiding this comment.
Cannot assign k8s secret to a Redis user maybe?
There was a problem hiding this comment.
Yes, you are right, thank you!
Fixed
| @@ -174,9 +174,9 @@ func (r *RedisUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( | |||
| userID := fmt.Sprintf(instaclustr.RedisUserIDFmt, clusterID, user.Namespace) | |||
| err = r.API.DeleteRedisUser(userID) | |||
| if err != nil { | |||
| l.Error(err, "Cannot delete Redis user", "user", user.Name) | |||
| l.Error(err, "Cannot delete Redis user on Instaclustr", "user", user.Name) | |||
There was a problem hiding this comment.
What does Cannot delete Redis user on Instaclustr mean? We delete users from a cluster, isn't it?
There was a problem hiding this comment.
Yes, fixed
| return err | ||
| } | ||
|
|
||
| controllerutil.RemoveFinalizer(user, user.DeletionUserFinalizer(clusterID, user.Namespace)) |
There was a problem hiding this comment.
DeletionUserFinalizer -> DeletionFinalizer
There was a problem hiding this comment.
It's a separate method for user, DeletionFinalizer used for secrets
| for _, ref := range redis.Spec.UserRefs { | ||
| err = r.detachUserResource(ctx, logger, redis, ref) | ||
| if err != nil { | ||
| logger.Error(err, "Cannot detach Redis user cluster", |
There was a problem hiding this comment.
Cannot detach Redis user cluster -> Cannot detach Redis user
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
from
3dd3402
to
9430e84
Compare
testisnullus
requested changes
Jul 20, 2023
| @@ -174,9 +174,9 @@ func (r *RedisUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( | |||
| userID := fmt.Sprintf(instaclustr.RedisUserIDFmt, clusterID, user.Namespace) | |||
| err = r.API.DeleteRedisUser(userID) | |||
| if err != nil { | |||
| l.Error(err, "Cannot delete Redis user", "user", user.Name) | |||
| l.Error(err, "Cannot delete Redis user on cluster. Cluster ID: %s.", "user", clusterID, user.Name) | |||
There was a problem hiding this comment.
Cannot delete Redis user on cluster -> Cannot delete Redis user from the cluster
|
|
||
| l.Error(err, "Cannot get Redis user", "user", u.Spec) | ||
| r.EventRecorder.Eventf(redis, models.Warning, models.DeletionFailed, | ||
| "Cannot get Redis user. user reference: %v", uRef) |
There was a problem hiding this comment.
Cannot get Redis user. user reference -> Cannot get Redis user. User reference
OleksiienkoMykyta
force-pushed
the
issue-469-delete-all-users-from-deleted-cluster
branch
from
9430e84
to
53cbdd7
Compare
testisnullus
approved these changes
Jul 20, 2023
taaraora
approved these changes
Jul 21, 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.
No description provided.