Skip to content
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

[BUG] wash app commands do not respect wash context #954

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

ahmedtadde
Copy link
Contributor

Feature or Problem

When a context is set, wash app commands should respect that context

Related Issues

#864

Release Information

wash-cli v0.22.0

Consumer Impact

improved correctness and DX for wash app commands.

Testing

Unit Test(s)

test_lattice_prefix_and_nats_client_from_cmd_opts

…x arg > context arg > $current_default context.lattice_prefix)

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
@ahmedtadde ahmedtadde requested a review from a team as a code owner November 7, 2023 18:05
Copy link
Contributor

@connorsmith256 connorsmith256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just requesting a couple small refactors 🙏

crates/wash-cli/src/app/mod.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/mod.rs Outdated Show resolved Hide resolved
…_prefix parsing on app cmds

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
Signed-off-by: Ahmed <ahmedtadde@gmail.com>
Signed-off-by: Ahmed <ahmedtadde@gmail.com>
@connorsmith256
Copy link
Contributor

@ahmedtadde the latest changes look great. It looks like part of your new test is failing, but only on Windows. I suspect set_default_context might not be working on Windows(?) If you add #[cfg(not(target_family = "windows"))] above the new test and it passes, I think we should merge this and file a separate issue to investigate wash contexts on Windows

@connorsmith256
Copy link
Contributor

Filed the suspected windows bug: #966

@connorsmith256 connorsmith256 merged commit 9da236f into wasmCloud:main Nov 8, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants