-
Notifications
You must be signed in to change notification settings - Fork 5
fix(#38): add sa_token_encrypted to cluster model #40
Conversation
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
+ Coverage 75.79% 76.12% +0.32%
==========================================
Files 13 13
Lines 752 758 +6
==========================================
+ Hits 570 577 +7
+ Misses 150 148 -2
- Partials 32 33 +1
Continue to review full report at Codecov.
|
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.
Overall looks good but let's modify the code a bit.
- "service-account-token-encrypted" in configuration JSON should be optional. If not set then use "true" be default. So, we don't have to modify the existing configuration secret.
- I don't see any tests which checks if this flag is set to false. It's set to true in all tests or I'm missing something? We need to add tests for:
- Configuration is set to "true", "false", not-set. Check it's loaded properly.
- When creating an entry in the DB the flag is honored (maybe you already have a test for that though).
@alexeykazakov I have addressed your comments. |
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.
In the parent issue description, you mentioned that the default value for the new field/column should be false
but here in this PR, it's true
. Also, I'm not sure I understand how this new field/column is used at all.
Lastly, please don't use the NOT NULL
constraint on the new column now, otherwise we can't rollback the deployment. We can add this constraint in a later PR/deployment, though
configuration/configuration.go
Outdated
@@ -613,3 +619,8 @@ func (c *ConfigurationData) GetDevModePrivateKey() []byte { | |||
} | |||
return nil | |||
} | |||
|
|||
// IsSaTokenEncrypted return pointer to bool | |||
func IsSaTokenEncrypted(encrypted bool) *bool { |
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.
the name of this function is confusing: you just return a pointer to the given bool, don't you?
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.
ok, let me change that to PointerToBool
?
migration/migration_blackbox_test.go
Outdated
|
||
// check if ALL the existing rows & new rows have the default value | ||
rows, err := sqlDB.Query("SELECT sa_token_encrypted FROM cluster") | ||
if err != nil { |
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.
require.NoError(t, err)
@@ -0,0 +1,2 @@ | |||
-- Store sa_token_encrypted for cluster | |||
ALTER TABLE cluster ADD COLUMN sa_token_encrypted boolean NOT NULL DEFAULT FALSE; |
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.
please don't use NOT NULL
in this sql statement: if we need to rollback the deployment, we'll be in troubles.
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 don't see reason to rollback the deployment. Do you see?
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's just a good practice IMO
@xcoulon I have updated issue description. Sorry for confusion. Let me know if it's clear to you |
Fixes: #38