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
Display clouds integration test #11072
Display clouds integration test #11072
Conversation
6234651
to
c0e2685
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.
High level looks good. Some comments related to maintainability. :-)
Provided QA was successful. How do I test the runner formatting changes?
fi | ||
|
||
cp ./tests/suites/cli/clouds/public-clouds.yaml "${TEST_DIR}"/juju/public-clouds.yaml | ||
OUT=$(XDG_DATA_HOME="${TEST_DIR}" juju clouds --local --format=json 2>/dev/null | jq ".[] | select(.defined != \"built-in\")") |
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.
Please add an echo of command run, or what's being attempted, for easier debugging and review of test results.
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.
Running: ./main.sh -V cli test_desplay_clouds
(notice capital V
) will tell you exactly what's running... It's easier to see what's going on.
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.
Okay that worked, but not exactly human reading friendly.
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 wonder if we can do something about that and only trace out certain things, let me think about this.
EOF | ||
) | ||
|
||
OUT=$(XDG_DATA_HOME="${TEST_DIR}" juju clouds --all --format=json 2>/dev/null | jq ".[] | select(.defined != \"built-in\")") |
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.
Please add an echo of command run, or what's being attempted, for easier debugging and review of test results.
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.
Running: ./main.sh -V cli test_desplay_clouds
(notice capital V
) will tell you exactly what's running... It's easier to see what's going on.
!!build!! |
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! Thank you
|
The following changes introduce the idea of a display clouds integration tests. Unfortunately it required to change how we handle machine interactions. For formatting directives such as json or yaml, we never want to be prompted for anything, we just either want an empty output in the right format (json being {}) or the data. Bringing in the changes is the first drive into doing so, the PR doesn't enforce the changes through out, although it is a first step in doing so. Adding redirection of the stderr to /dev/null for machine directives is pointless, so in the future checking for ctx.IsSerial() should be used. If the value is true then omitting the information and the noise is probably the wisest.
The following updates the test to use a real (read as "test") public cloud. That way it becomes more obvious what the test meant to do. The changes introduce a fake openstack test cloud with fake urls for the regions.
The following changes uses a real public cloud rather than some fake canonical one.
The following adds a reason for the introduction of IsSerial. The basic idea is that we want to provide a more holistic approach to handling the formatting directives in the CLI without having to resorting to magic string checking.
45ca26d
to
6815d74
Compare
The following updates show-storage so that it's backwards compatible with the feature tests around it. Essentially the fact that the user asked for formatted directive of yaml and instead was returned a string doesn't match what they asked for. The fix here is to say, return an empty object `{}` (remembering that yaml is a subset of json) and then correctly outputting the issue to stderr. The consumer of the CLI can then either check stderr for issues or redirect stderr to a file or /dev/null. Either way the consumer gets valid yaml and you also get the correct information about why it failed. See: https://discourse.jujucharms.com/t/cli-serialisable-output-formats/2490
The following cleans up the double logging from storage show command. It seemed ridiculous for every command to return an error and log the same error. To clean this up, the command package is now again opinionated correctly and we've followed suit with the commands.
|
|
Checklist
Description of change
The following changes introduce the idea of a display clouds integration
tests. Unfortunately it required to change how we handle machine
interactions. For formatting directives such as json or yaml, we never
want to be prompted for anything, we just either want an empty output in
the right format (json being {}) or the data.
Bringing in the changes is the first drive into doing so, the PR doesn't
enforce the changes through out, although it is a first step in doing
so.
Adding redirection of the stderr to
/dev/null
for machine directives ispointless, so in the future checking for
ctx.IsSerial()
should be used.If the value is true then omitting the information and the noise is
probably the wisest.
See: https://discourse.jujucharms.com/t/cli-serialisable-output-formats/2490
QA steps
Please replace with how we can verify that the change works?