CLOUDP-332913: [AtlasCLI] Decouple profile from viper #4050
Conversation
internal/validate/validate_test.go
Outdated
| @@ -141,6 +141,7 @@ func TestObjectID(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| /* | |||
There was a problem hiding this comment.
instead of commenting the test you can add t.Skip("Will reenable on ticket CLOUDP-XXX")
There was a problem hiding this comment.
Done, thanks for the suggestion!
internal/validate/validate_test.go
Outdated
| t.Fatalf("NoAPIKeys() expected error\n") | ||
| } | ||
| }) | ||
| /* |
| // Configuration needs to be deleted from toml, as viper doesn't support this yet. | ||
| // FIXME :: change when https://github.com/spf13/viper/pull/519 is merged. |
There was a problem hiding this comment.
[nit] can we add a cloudp ticket in backlog so we don't forget about this?
There was a problem hiding this comment.
This comment was there since 2018, I didn't want to change the code to make the PR easier to review.
There was a problem hiding this comment.
ah wow, I am wondering if it is still valid 😅
| // Configuration needs to be deleted from toml, as viper doesn't support this yet. | ||
| // FIXME :: change when https://github.com/spf13/viper/pull/519 is merged. |
| } | ||
|
|
||
| func (*InMemoryStore) RenameProfile(_, _ string) error { | ||
| panic("not implemented") |
There was a problem hiding this comment.
I am wondering what is the benefit here of using panic instead of returning the error as the function signature suggests to do 🤔
There was a problem hiding this comment.
InMemoryStore is not meant to be used, it's just there to keep tests working. The ultimate goal is to get rid of it. So if someone creates a new test they will realize they shouldn't use the method.
| if !exists { | ||
| if err := s.fs.MkdirAll(s.configDir, defaultPermissions); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return s.viper.WriteConfigAs(s.Filename()) |
There was a problem hiding this comment.
I think is more readable but feel free to ignore
| if !exists { | |
| if err := s.fs.MkdirAll(s.configDir, defaultPermissions); err != nil { | |
| return err | |
| } | |
| } | |
| return s.viper.WriteConfigAs(s.Filename()) | |
| if exists { | |
| return s.viper.WriteConfigAs(s.Filename()) | |
| } | |
| return s.fs.MkdirAll(s.configDir, defaultPermissions) |
There was a problem hiding this comment.
Will create a ticket to address the actual logic, right now it's just a copy pasted
Co-authored-by: Melanija Cvetic <[email protected]>
jeroenvervaeke
force-pushed
the
CLOUDP-332913
branch
from
e0ba606 to
d0d1323
Compare
|
Coverage Report 📉
|
Co-authored-by: Melanija Cvetic <[email protected]>
Proposed changes
config.Store(originally calledConfigStore, but the linter didn't like it)ViperConfigStore, I tried to not touch the logic, mainly copy paste where possibleWithProfile/ProfileFromContextand attached the profile to the context, so in commands you can get theProfilefrom thecontextinstead of using static variables.profile.gointoprofile.go/identifiers.go(code was unrelated to profile)internal/validate/validate_test.gobecause they directly invoke viper 👎 , will fix them in a followup PRJira ticket: CLOUDP-332913