Add ctx command #455
Add ctx command #455
Conversation
sttts
commented
Mar 27, 2024
•
sttts
commented
sttts
force-pushed
the
sttts-poc-ctx
branch
7 times, most recently
from
587ebe6
to
778f75a
Compare
sttts
force-pushed
the
sttts-poc-ctx
branch
3 times, most recently
from
87f677c
to
eb4042f
Compare
sttts
force-pushed
the
sttts-poc-ctx
branch
2 times, most recently
from
4715dff
to
c024f97
Compare
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
There was a problem hiding this comment.
Thanks @sttts ! Generally looks good to me. I'm not seeing anything fundamentally blocking, so I'm ok with moving forward with this and addressing my comments as follow ups 👍 (windows support, co-locating/isolating the kubeconfig interactions).
|
|
||
| case tea.KeyMsg: | ||
| switch keypress := msg.String(); keypress { | ||
| case "ctrl+c", "esc": |
There was a problem hiding this comment.
Do we detect OS anywhere? If not we may need to since we have binaries being produced for mac, linux, and windows and this keypress can vary in expectation.
There was a problem hiding this comment.
Aren't ctrl-c and esc universal?
There was a problem hiding this comment.
Isn't ctrl c in windows (copy) or did that change?
There was a problem hiding this comment.
Maybe this is outdated. It's been awhile since I've worked in a windows environment 🤷♂️
There was a problem hiding this comment.
@cwilhit can you test the command with Windows (can be after merge) ?
There was a problem hiding this comment.
Yes, absolutely. ctrl + c sends SIGINT on Windows
| kongCtx.Bind(upCtx) | ||
|
|
||
| if !upCtx.Profile.IsSpace() { | ||
| // TODO: add legacy support |
There was a problem hiding this comment.
This comment is instead in reference to SaaS rather than legacy?
There was a problem hiding this comment.
yes, SaaS. But we will rework all commands soonish, this included to talk to the Space API of cloud.
| if err := os.MkdirAll(dir, 0755); err != nil { // nolint:gosec // it's ok | ||
| return errors.Wrap(err, "failed to create parent directories") | ||
| } | ||
| return os.WriteFile(path, []byte(value), 0644) // nolint:gosec // it's ok |
There was a problem hiding this comment.
Is this state file just tracking a single string?
There was a problem hiding this comment.
yes, the code is taken from kubectx.
| if err != nil { | ||
| return "", err | ||
| } | ||
| if err := writeLastContext(prevContext); err != nil { // nolint:staticcheck |
There was a problem hiding this comment.
Something that's not quite clear yet, if we're storing the previous context in the kubeconfig, why do we also need to store its name in the ctx state file?
There was a problem hiding this comment.
To make kubectl ctx - work.
| return nil | ||
| } | ||
|
|
||
| func (c *Cmd) RunSwap(ctx context.Context, upCtx *upbound.Context) error { // nolint:gocyclo // TODO: shorten |
There was a problem hiding this comment.
Between some of the logic I'm seeing in mergeIntoKubeConfig and the logic in here, I wonder if it would be beneficial to move these operations to the kubeconfig package and add functions like (simply examples):
- Swap(a, b)
- MergeInto(cfg, new, prev)
And then we can test those operations on their own and simplify some of these calling functions.
There was a problem hiding this comment.
I got more confused by the shared functions that exist, but are actually pretty special. I don't think these functions are suitable really for being shared.
There was a problem hiding this comment.
I see. When I was reading through the operations, I mostly had an urge to:
- Not want to have to jump through these hopes somewhere else 😅
- Add tests specific to the operations - if for nothing else to ensure if something changes they fail.
There was a problem hiding this comment.
Added lots of tests to the swap command. It was needed 😇
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
