fix(#38): add sa_token_encrypted to cluster model #40
Conversation
dipak-pawar
requested review from
alexeykazakov and
xcoulon
and removed request for
alexeykazakov
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.
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.
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.
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.
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 { |
| @@ -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.
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.
I don't see reason to rollback the deployment. Do you see?
There was a problem hiding this comment.
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