-
Notifications
You must be signed in to change notification settings - Fork 26
Add use-context command #96
Add use-context command #96
Conversation
.github/workflows/e2e.yml
Outdated
# fails if command succeeds | ||
! layerform config select-context test-does-not-exist # context does not exist | ||
|
||
layerform config set-context test-local -t local --dir test && layerform config select-context test-local |
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'm still wondering what is the default layerform/configurations.yml file in our CI/CD testing env.
As the mitigation to ensure that a config exists, I do set-context
first to create a config.
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.
Well, it is the last one that run in the previous step (check line 50).
At this point, the current context is the test-local
, your set-context
ends up being a no-op
.
What I think you should do to test is somehow check if the use-context
actually works.
You can use the config get-contexts
command alongside with grep
for that. The selected
context will have a *
in it's line.
I'll make a comment with suggestions.
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.
updated 9e15b35
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.
Hi @rafiramadhana,
Thank you so much for the contribution, overall the implementation seems correct.
I think you got confused with the name of the command, the idea was to call it use-context
. Do you mind changing it? Also change the name of the file and stuff.
cmd/cli/select_context.go
Outdated
os.Exit(1) | ||
} | ||
|
||
fmt.Fprintf(os.Stdout, "Context \"%s\" selected.\n", name) |
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.
Can we change this to:
fmt.Fprintf(os.Stdout, "Context \"%s\" selected.\n", name) | |
fmt.Fprintf(os.Stdout, "Switched to context \"%s\".\n", name) |
FYI I'm getting inspiration from kubectl
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.
updated 978e3c6
cmd/cli/select_context.go
Outdated
if !ok { | ||
fmt.Fprintf( | ||
os.Stderr, | ||
"context %s does not exist\n", |
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.
Can we change this to:
"context %s does not exist\n", | |
"no context exists with the name \"%s\".\n", |
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.
updated 978e3c6
Update e2e test of use context.
b41aede
to
9e15b35
Compare
@vieiralucas it looks like the configure s3 pipeline is broken
https://github.com/ergomake/layerform/actions/runs/6229934241/job/16909208767?pr=96 could this be related to my PR changes? |
Hi @rafiramadhana, this happens because your changes to the tests leave a context of type |
.github/workflows/e2e.yml
Outdated
# switch context to test-local | ||
layerform config use-context test-local | ||
layerform config get-contexts | tee usecontext | ||
grep -E '^\*\s+test-local' usecontext | ||
! grep -E '^\*\s+test-s3' usecontext | ||
|
||
# switch context to test-s3 | ||
layerform config use-context test-s3 | ||
layerform config get-contexts | tee usecontext | ||
! grep -E '^\*\s+test-local' usecontext | ||
grep -E '^\*\s+test-s3' usecontext |
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 think this change should "fix" the tests
# switch context to test-local | |
layerform config use-context test-local | |
layerform config get-contexts | tee usecontext | |
grep -E '^\*\s+test-local' usecontext | |
! grep -E '^\*\s+test-s3' usecontext | |
# switch context to test-s3 | |
layerform config use-context test-s3 | |
layerform config get-contexts | tee usecontext | |
! grep -E '^\*\s+test-local' usecontext | |
grep -E '^\*\s+test-s3' usecontext | |
# switch context to test-s3 | |
layerform config use-context test-s3 | |
layerform config get-contexts | tee usecontext | |
! grep -E '^\*\s+test-local' usecontext | |
grep -E '^\*\s+test-s3' usecontext | |
# switch context to test-local | |
layerform config use-context test-local | |
layerform config get-contexts | tee usecontext | |
grep -E '^\*\s+test-local' usecontext | |
! grep -E '^\*\s+test-s3' usecontext |
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.
updated 3f94a83
thanks for the pointers
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.
Awesome @rafiramadhana
Thank you so much for this <3
Why this matters
Resolves #89
Solution
Add command
use-context
to use the selected current context.How to test it
Run the e2e.yml.
Note to reviewers