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.

Sign up for GitHub

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

Jump to bottom

[JUJU-1209] Added a bash-based version of the unregister-controller test. #14538

Merged
merged 4 commits into from
Sep 6, 2022
Merged

[JUJU-1209] Added a bash-based version of the unregister-controller test. #14538

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc4ff33
Added bash-based unregister-controller test.
anvial Aug 30, 2022
cda592d
Removed autogenerated comments from expect scripts.
anvial Aug 30, 2022
573b34b
Added accounts.yaml backup/restore, that helped to avoid using expect…
anvial Aug 31, 2022
b0e5a61
Added check that default controller is not equal to unregistered one …
anvial Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/main.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ fi
echo ""

echo "==> Checking for dependencies"
check_dependencies curl jq yq shellcheck
check_dependencies curl jq yq shellcheck expect
Copy link
Member

Choose a reason for hiding this comment

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

README.md also needs to be updated:

## Getting started

Before running tests, you'll need to install `jq`, `yq`, `shellcheck` and `golangci-lint`:

sudo snap install jq
sudo snap install yq
sudo snap install shellcheck
go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.44.2

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if that exact version is still even correct anymore. I feel like this README entry should probably be redirected to something that is actually used by the CI suite as well, to ensure dependencies, so that we get them lined up, and can avoid future skew.

Copy link
Member Author

Choose a reason for hiding this comment

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

Backup/restore acoounts.yaml file helped to avoid using expect so we can postpone this tool introduction


if [[ ${USER:-'root'} == "root" ]]; then
echo "The testsuite must not be run as root." >&2
Expand Down
21 changes: 21 additions & 0 deletions tests/suites/controller/expect-scripts/juju-change-user-password.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/usr/bin/expect -f

set force_conservative 0 ;# set to 1 to force conservative mode even if
;# script wasn't run conservatively originally
if {$force_conservative} {
set send_slow {1 .1}
proc send {ignore arg} {
sleep .1
exp_send -s -- $arg
}
}

set timeout 10
spawn juju change-user-password
match_max 100000
expect -exact "new password: "
send -- "adminpass\r"
expect -exact "\r
type new password again: "
send -- "adminpass\r"
expect eof
19 changes: 19 additions & 0 deletions tests/suites/controller/expect-scripts/juju-login.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/usr/bin/expect -f

set force_conservative 0 ;# set to 1 to force conservative mode even if
;# script wasn't run conservatively originally
if {$force_conservative} {
set send_slow {1 .1}
proc send {ignore arg} {
sleep .1
exp_send -s -- $arg
}
}

set timeout 10
set controller_name [lindex $argv 1]
spawn juju login -u admin -c {*}$argv
Copy link
Member

Choose a reason for hiding this comment

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

this syntax seems a bit hard to understand, but I have the feeling we'll get used to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Backup/restore acoounts.yaml file helped to avoid using expect so we can postpone this tool introduction

match_max 100000
expect -exact "please enter password for admin on $controller_name: "
send -- "adminpass\r"
expect eof
2 changes: 2 additions & 0 deletions tests/suites/controller/task.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,7 @@ test_controller() {
# Leave this one last, as it can cause mongo to slowdown to a snails pace.
test_mongo_memory_profile

test_unregister

destroy_controller "test-controller"
}
46 changes: 46 additions & 0 deletions tests/suites/controller/unregister.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
run_unregister() {
echo

file="${TEST_DIR}/unregister.log"

ensure "unregister" "${file}"

controller_name=$(juju controllers --format=json | jq -r '."current-controller"')

echo "Change admin password"
Copy link
Member

Choose a reason for hiding this comment

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

At a high level, why is our "unregister test suite" worrying about change user password and login? (IOW, while this may be a faithful representation of our existing test suite, and it is great to port across, we should take a critical view and consider if we should be testing this behavior as part of this test.)

Maybe it should, as you test that you can login again to a controller that you have unregistered. (presumably this is testing that the controller hasn't actually been destroyed, just removed from local info?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Backup/restore acoounts.yaml file helped to avoid using juju login (and expect tool)

./tests/suites/controller/expect-scripts/juju-change-user-password.exp

echo "Backup controller info before unregister"
cp ~/.local/share/juju/controllers.yaml ~/.local/share/juju/controllers.yaml.bak

echo "Unregister controller"
juju unregister --yes "${controller_name}"

juju controllers --format=json | jq -r ".\"controllers\".\"${controller_name}\"" | check null
Copy link
Member

Choose a reason for hiding this comment

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

I think a comment here with:

# if you unregister the active controller, it should ensure that the current active controller is reset to empty

Copy link
Member

Choose a reason for hiding this comment

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

I do have a concern that this check doesn't really check what I think the behavior of unregister is meant to be.

The meaning of juju unregister is not 'I have unset the default controller'. The meaning is that "I no longer know who $controller is", whether they were the default or not.
So I'd rather see a test of:
juju controllers and that before the unregister it lists that the named controller is there, and after the unregister it no longer does.
I think it is fine to then also include "and if the controller that was removed was the default controller, we don't leave a default value that points to something that is now unknown".
But we are missing the bit that says "this is no longer a known controller".

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it,

I've added:

  • a check that the named controller is in juju controllers output before the unregister
  • a check that the default controller is not equal to the unregistered one after juju unregister.


echo "Restore controller info after unregister"
mv ~/.local/share/juju/controllers.yaml.bak ~/.local/share/juju/controllers.yaml

echo "Login to controller info after restoring"
./tests/suites/controller/expect-scripts/juju-login.exp "${controller_name}"
juju switch unregister

juju controllers --format=json | jq -r '."current-controller"' | check "${controller_name}"

destroy_model "unregister"
}

test_unregister() {
if [ -n "$(skip 'test_unregister')" ]; then
echo "==> SKIP: Asked to skip controller unregister tests"
return
fi

(
set_verbosity

cd .. || exit

run "run_unregister"
)
}