-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cli): retain config changes on re-login #2388
Conversation
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
47f762c
to
2f412a9
Compare
internal/cli/cmd/login/login.go
Outdated
// Merge new settings with the existing configuration | ||
o.Config.APIAddress = o.ServerAddress | ||
o.Config.BearerToken = bearerToken | ||
o.Config.RefreshToken = refreshToken | ||
o.Config.InsecureSkipTLSVerify = o.InsecureTLS |
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.
This looks very close, but I think that if the server we're logging into isn't the same as the one we were previously logged into, then we don't want to merge because we will want to lose any other server-specific settings in the config file at that point, e.g. default project.
Could we conditionally merge or start with a new config from scratch?
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.
Ah I think I see what you mean, I will adjust this now and hopefully fix it if I am understanding correctly
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.
Adjusted the PR now
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.
Looks great! Thanks!
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.
Actually... my apologies... this works, but something like the following pseudocode may be more efficient and easier to read and maintain:
if not same server:
cfg = new config
cfg.apiAddress = apiAddress
cfg.bearerToken = bearerToken
# etc...
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.
Agreed, that would definitely be easier to read. I've updated PR now, hopefully it makes sense - let me know if you think there's any comment(s) needed on it or if it's self-explanatory
2f412a9
to
f3ac23a
Compare
Signed-off-by: Michael Walsh <walshmichael310@gmail.com>
f3ac23a
to
b069f66
Compare
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.
LGTM @walsm232. Solid first contribution to the project. Thank you!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2388 +/- ##
=======================================
Coverage 47.63% 47.64%
=======================================
Files 244 244
Lines 17396 17395 -1
=======================================
Hits 8287 8287
+ Misses 8698 8697 -1
Partials 411 411 ☔ View full report in Codecov by Sentry. |
Fixes: #2363