test: pack test charms #18151

barrettj12 · 2024-09-27T19:25:41Z

Deploying a charm from an unpacked directory is no longer supported in 4.0. Hence, when using local charms in testing, we need to pack them first inside the test.

I added a test utility function pack_charm which uses Charmcraft to pack the charm at the given path, and returns a glob that can be provided to juju deploy to deploy the packed charm.
I have added/updated the charmcraft.yaml in several testing charms to be compatible with the new Charmcraft changes in 3.0.
I have added charmcraft to the checked dependencies for those tests which require charms to be packed.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
~~[ ] Go unit tests, with comments saying what you're testing~~
Integration tests, with comments saying what you're testing
~~[ ] doc.go added or updated in changed packages~~

QA steps

Run the following test suites: deploy, hooks, relations, sidecar, spaces_ec2, storage.

Many of these tests will not pass due to other issues, but at least we should not see the following error:

ERROR attempting to deploy "....": deploying from directory not supported

Links

Jira card: JUJU-6825

hmlanigan

We shouldn't be committing binaries to the code base.

Please prepare for Charmcraft 3.0 and ubuntu 24.04 where each Charmcraft.yaml can have only 1 base. One issue to solve will to be to determine which architecture to build for the test run.

testcharms/charms/dummy-storage-fs/charmcraft.yaml

Deploying a charm from an unpacked directory is no longer supported in 4.0. Hence, when using local charms in testing, we need to pack them first inside the test. - I added a test utility function `pack_charm` which uses Charmcraft to pack the charm at the given path, and returns a glob that can be provided to `juju deploy` to deploy the packed charm. - I have added/updated the `charmcraft.yaml` in several testing charms to be compatible with the new Charmcraft changes in 3.0. - I have added `charmcraft` to the checked dependencies for those tests which require charms to be packed.

barrettj12 · 2024-10-09T15:02:06Z

/build

Aflynn50

Looks good, just a question about errors in the tests

Aflynn50 · 2024-10-10T09:10:59Z

tests/suites/spaces_ec2/juju_bind.sh

@@ -22,7 +22,8 @@ run_juju_bind() {
 	configure_multi_nic_netplan "$juju_machine_id" "$hotplug_iface"

 	# Deploy test charm to dual-nic machine
-	juju deploy ./testcharms/charms/space-defender --bind "defend-a=alpha defend-b=isolated" --to "${juju_machine_id}"
+	# shellcheck disable=SC2046


What happens with the output of pack_charm here if it fails for some reason? Does doing it in line like this hide the error in any way? If it does we should do it as a separate line cuz it'll make the tests a lot easier to debug

I'm not sure, can you test it?

jack-w-shaw · 2024-10-10T12:40:14Z

tests/includes/charmcraft.sh

+
+	charmcraft pack -p "$CHARM_DIR"
+	echo "./${CHARM_NAME}_*.charm"
+}


It would be really great if you could cache charms on the host file system. That way we can save a bunch of time when running tests locally

Charmcraft internally caches a lot of stuff, so packing charms is way quicker on subsequent runs, like 15 seconds.

Yeah fair enough. It would still be nice though!

@barrettj12 is correct here.

Do we know if it uses lxd by default, if not, it might be worth testing that using charmcraft pack --use-lxd. If it is, just ignore me :)

Yes, it uses LXD by default. You have to pass --destructive-mode to build on the host.

jack-w-shaw · 2024-10-10T12:41:02Z

tests/suites/deploy/deploy_charms.sh

+	# shellcheck disable=SC2046
+	juju deploy $(pack_charm ./testcharms/charms/lxd-profile)
+	# shellcheck disable=SC2046
+	juju deploy $(pack_charm ./testcharms/charms/lxd-profile-subordinate)


I'd rather we quote properly than add shellcheck exceptions

Quoting doesn't work because the output of pack_charm is a glob (e.g. lxd-profile_*.charm). If it is quoted, it will be treated like a literal file name, which is not what we want here.

jack-w-shaw · 2024-10-10T12:41:26Z

tests/suites/deploy/deploy_charms.sh

-	charm=./tests/suites/deploy/charms/lxd-profile-alt
-	juju deploy "${charm}" --to lxd
+	charm=$(pack_charm ./tests/suites/deploy/charms/lxd-profile-alt)
+	juju deploy ${charm} --to lxd


Why remove quotes?

See above reply.

#18211 The local charm integration tests relied on the ntp charm which was not up to date with the latest charm requrements and caused an error on deploy. It was held in launchpad and not under our control. Switch to using the ubuntu-plus charm instead from test testcharms directory. Note that these tests deploy from directory so will not work in 4.0. They will need to be updated or maybe even just deleted when the patch is merged up. If they are kept they should use the work added in #18151. BONUS COMMIT: add big V verbose option   ## Checklist  - [x] Code style: imports ordered, good names, simple structure, etc - [x] Comments saying why design decisions were made - [x] Go unit tests, with comments saying what you're testing - [x] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing - [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages ## QA steps ``` ./main.sh -v cli ```

Haven't actually QAed yet

SimonRichardson · 2024-10-11T14:29:09Z

tests/includes/charmcraft.sh

+
+	charmcraft pack -p "$CHARM_DIR"
+	echo "./${CHARM_NAME}_*.charm"
+}


@barrettj12 is correct here.

Do we know if it uses lxd by default, if not, it might be worth testing that using charmcraft pack --use-lxd. If it is, just ignore me :)

SimonRichardson · 2024-10-11T14:29:43Z

tests/suites/deploy/deploy_charms.sh

+		echo "==> Checking for dependencies"
+		check_dependencies charmcraft


SimonRichardson · 2024-10-11T14:35:43Z

tests/includes/charmcraft.sh

+	local CHARM_DIR=$1
+	CHARM_NAME=$(basename "$CHARM_DIR")
+
+	charmcraft pack -p "$CHARM_DIR"


If you want to see the messages and we want to ensure it fails correctly, even when inline you could do the following.

Suggested change

charmcraft pack -p "$CHARM_DIR"

set -eux

charmcraft pack -p "$CHARM_DIR"

set_verbosity

PR has changed

SimonRichardson · 2024-10-11T16:38:53Z

Note: we should ensure that the Jenkin runners have charmcraft installed.

barrettj12 · 2024-10-15T13:40:12Z

/merge

barrettj12 requested a review from hmlanigan September 27, 2024 19:50

hpidcock added the 4.0 label Sep 30, 2024

hmlanigan previously requested changes Sep 30, 2024

View reviewed changes

testcharms/charms/dummy-storage-fs/charmcraft.yaml Outdated Show resolved Hide resolved

barrettj12 requested a review from hmlanigan October 2, 2024 16:42

barrettj12 mentioned this pull request Oct 3, 2024

fix: checking for hooks in local charm #18173

Merged

2 tasks

barrettj12 requested a review from Aflynn50 October 7, 2024 13:41

hpidcock added the has merge conflicts label Oct 8, 2024

barrettj12 requested a review from jack-w-shaw October 9, 2024 14:25

barrettj12 force-pushed the pack-testcharms branch from dacae14 to ded0fe2 Compare October 9, 2024 14:41

Aflynn50 approved these changes Oct 10, 2024

View reviewed changes

Aflynn50 mentioned this pull request Oct 10, 2024

test: fix local charm integration tests #18211

Merged

5 tasks

jack-w-shaw reviewed Oct 10, 2024

View reviewed changes

barrettj12 removed the has merge conflicts label Oct 10, 2024

anvial requested a review from SimonRichardson October 11, 2024 14:18

jack-w-shaw previously approved these changes Oct 11, 2024

View reviewed changes

SimonRichardson reviewed Oct 11, 2024

View reviewed changes

hmlanigan removed their request for review October 11, 2024 15:25

SimonRichardson approved these changes Oct 11, 2024

View reviewed changes

jujubot merged commit 8022334 into juju:main Oct 15, 2024
16 of 17 checks passed

barrettj12 deleted the pack-testcharms branch October 15, 2024 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: pack test charms #18151

test: pack test charms #18151

barrettj12 commented Sep 27, 2024 •

edited

Loading

hmlanigan left a comment

barrettj12 commented Oct 9, 2024

Aflynn50 left a comment

Aflynn50 Oct 10, 2024

barrettj12 Oct 10, 2024

jack-w-shaw Oct 10, 2024

barrettj12 Oct 10, 2024

jack-w-shaw Oct 11, 2024 •

edited

Loading

SimonRichardson Oct 11, 2024

barrettj12 Oct 11, 2024

jack-w-shaw Oct 10, 2024

barrettj12 Oct 10, 2024

jack-w-shaw Oct 11, 2024

jack-w-shaw Oct 10, 2024

barrettj12 Oct 10, 2024

SimonRichardson Oct 11, 2024

SimonRichardson Oct 11, 2024

SimonRichardson Oct 11, 2024

SimonRichardson commented Oct 11, 2024

barrettj12 commented Oct 15, 2024

		echo "==> Checking for dependencies"
		check_dependencies charmcraft

test: pack test charms #18151

test: pack test charms #18151

Conversation

barrettj12 commented Sep 27, 2024 • edited Loading

Checklist

QA steps

Links

hmlanigan left a comment

Choose a reason for hiding this comment

barrettj12 commented Oct 9, 2024

Aflynn50 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-w-shaw Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson commented Oct 11, 2024

barrettj12 commented Oct 15, 2024

barrettj12 commented Sep 27, 2024 •

edited

Loading

jack-w-shaw Oct 11, 2024 •

edited

Loading