PTEUDO1071 - migrate new schemas users when version shape or type changes, delete CR #267
Conversation
…ew-schemas-users-when-version-shape-or-type-changes
EvertonCalgarotto
changed the title
| @@ -48,7 +48,7 @@ type Exister interface { | |||
| } | |||
|
|
|||
| type Remover interface { | |||
| DeleteUser(username, reassignToUser string) error | |||
| DeleteUser(dbName, username, reassignToUser string) error | |||
There was a problem hiding this comment.
| DeleteUser(dbName, username, reassignToUser string) error | |
| DeleteUser(ctx context.Context, dbName, username, reassignToUser string) error |
There was a problem hiding this comment.
Why is that? We don't use the context here for anything.
pkg/roleclaim/dbroleclaim_test.go
Outdated
| viperObj.Set("defaultMasterPort", "5432") | ||
| viperObj.Set("defaultSslMode", "require") | ||
| viperObj.Set("defaultMinStorageGB", "10") | ||
| viperObj.Set("defaultSslMode", "disable") |
There was a problem hiding this comment.
In general, viper has burned me when calling it inside libraries.
My suggestion in big refactors like this is to move all of the properties to a config object and read it from there. viper shouldn't be called outside of the main func at startup time. If we need some sort of hot reloading, that's different. We can register a method that viper can call to update properties though. It saves us from calling into viper anywhere in the package.
There was a problem hiding this comment.
When it's in the main code, I agree. These default values should be read when starting the app and kept somewhere in the context, to be reused when needed. But these are tests only, so I wouldn't bother too much about these in here.
pkg/roleclaim/roleclaim.go
Outdated
| if dbClaim.Spec.Host != "" { | ||
| return dbClaim.Spec.Host | ||
| } | ||
| return r.Config.Viper.GetString(fmt.Sprintf("%s::Host", dbcBaseConfig.FragmentKey)) |
There was a problem hiding this comment.
Same comment as above, we should not rely on viper inside libraries.
There was a problem hiding this comment.
alright, I'll think of something to load these values when the app is starting and keep them in the context or somewhere more meaningful then viper itself.
There was a problem hiding this comment.
put all viper methods in the basefunctions class to keep them centralized and easy to find.
| func (r *DbRoleClaimReconciler) matchInstanceLabel(dbClaim *v1.DatabaseClaim) (string, error) { | ||
| settingsMap := r.Config.Viper.AllSettings() | ||
|
|
||
| rTree := radix.New() | ||
| for k := range settingsMap { | ||
| if k != "passwordconfig" { | ||
| rTree.Insert(k, true) | ||
| } | ||
| } | ||
|
|
||
| // Find the longest prefix match | ||
| m, _, ok := rTree.LongestPrefix(dbClaim.Spec.InstanceLabel) | ||
| if !ok { | ||
| return "", fmt.Errorf("can't find any instance label matching fragment keys") | ||
| } | ||
|
|
||
| dbClaim.Status.ActiveDB.MatchedLabel = m | ||
|
|
||
| return m, nil | ||
| } |
There was a problem hiding this comment.
2 birds with one stone. If you remove this method, you can also remove the radix package. Modern CRs doesn't rely on this instanceLabel work.
There was a problem hiding this comment.
will remove it on another task
EvertonCalgarotto
deleted the
PTEUDO-1071-migrate-new-schemas-users-when-version-shape-or-type-changes
branch
Migrate schemas and users when version, shape or type changes
Delete users when DBRoleClaim is deleted
Create new users and schemas when DBRoleClaim is created.
Ensure passwords are rotated.