feat: add --dry-run flag to prepare and restore commands by tonyandrewmeyer · Pull Request #147 · canonical/concierge

tonyandrewmeyer · 2026-01-31T06:06:55Z

Adds a --dry-run flag that outputs what would happen (in a format that can be copy & pasted into a shell where possible) without making system changes.

Creates DryRunWorker that skips execution but delegates reads
Adds --dry-run flag to prepare and restore commands
Bumps the default logging level in dry-run mode to error
Adds fmt.Fprintln calls in DryRunWorker where execution is skipped

Fixes #61

Add a --dry-run flag that outputs what would happen without making system changes. Uses a hybrid approach where handlers announce intentions via Print() method, and DryRunWorker skips execution while delegating read operations to the real system for accurate conditional logic. - Add Print() method to Worker interface - Create DryRunWorker that skips execution but delegates reads - Add --dry-run flag to prepare and restore commands - Suppress logging output in dry-run mode - Add Print() calls throughout handlers for human-readable output - Add unit tests for dry-run functionality

- Fix manager_test.go to properly test DryRunWorker instead of MockSystem - Add integration tests for dry-run prepare and restore commands - Document in README that dry-run reads actual system state for accuracy Co-authored-by: tonyandrewmeyer <826522+tonyandrewmeyer@users.noreply.github.com>

feat: add dry-run option to prepare and restore (outstanding review comments)

…print lines. This also gives more similar 'you would execute this thing' type output, which I think is probably better.

The dry-run spread tests expected "Installing snap" and "Removing snap" messages, but after the refactoring to auto-print from DryRunWorker, the output now shows "Would run: snap install ..." and "Would run: snap remove ...". Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

1. Load cached runtime config in dry-run restore mode for accuracy. When a user runs `prepare --extra-snaps=foo` followed by `restore --dry-run`, they now see that foo would be removed. Falls back to current config if no cached config exists. 2. Add Print() calls when bootstrapping/destroying Juju controllers. Previously only printed when skipping operations, now also prints when actually performing them for consistent dry-run output. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

The test now actually verifies that DryRunWorker delegates read operations to the underlying system: - User() returns data from the mock system - ReadFile() and ReadHomeDirFile() return mocked file contents - SnapInfo() and SnapChannels() return mocked snap data Also fixes: - Add missing 'path' import to dryrun.go (required after linter change) - Initialize mockSnapChannels map in NewMockSystem() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

internal/snapd/client_test.go

tonyandrewmeyer · 2026-01-31T06:12:17Z

Parts of this were done with Claude Code (transcript), with a bunch of back-and-forth from me, and some hand-coding along the way. I then did a few series of reviews with Copilot in my fork, so am not bothering to do more of that here.

tonyandrewmeyer · 2026-01-31T06:17:54Z

Example run:

ubuntu@concierge-dry-run:~/concierge$ sudo go run ./main.go prepare -p dev --dry-run
Would run: /usr/bin/apt-get update
Would run: /usr/bin/apt-get install -y gnome-keyring
Would run: /usr/bin/apt-get install -y python3-pip
Would run: /usr/bin/apt-get install -y python3-venv
Would run: /usr/bin/snap install jhack --channel latest/stable
Would run: /usr/bin/snap connect jhack:dot-local-share-juju
Would run: /usr/bin/snap install charmcraft --channel latest/stable --classic
Would run: /usr/bin/snap install jq --channel latest/stable
Would run: /usr/bin/snap install yq --channel latest/stable
Would run: /usr/bin/snap install rockcraft --channel latest/stable --classic
Would run: /usr/bin/snap install snapcraft --channel latest/stable --classic
Preparing LXD provider
Preparing K8s provider
Would run: /usr/bin/which iptables
Would run: /usr/bin/snap install k8s --channel 1.32-classic/stable --classic
Would run: /usr/bin/snap install lxd
Would run: /usr/sbin/lxd waitready --timeout 270
Would run: /usr/sbin/lxd init --minimal
Would run: /usr/sbin/lxc network set lxdbr0 ipv6.address none
Would run: /usr/bin/chmod a+wr /var/snap/lxd/common/lxd/unix.socket
Would run: /usr/sbin/usermod -a -G lxd ubuntu
Would run: /usr/sbin/iptables -F FORWARD
Would run: /usr/sbin/iptables -P FORWARD ACCEPT
Would run: /usr/bin/snap install kubectl --channel stable --classic
Would run: /usr/bin/systemctl is-active containerd.service
Would remove: /run/containerd
Would run: k8s status
Would run: k8s status --wait-ready --timeout 270s
Would run: k8s set load-balancer.l2-mode=true
Would run: k8s set load-balancer.cidrs=10.43.45.0/28
Would run: k8s enable load-balancer
Would run: k8s enable local-storage
Would run: k8s enable network
Would run: k8s kubectl config view --raw
Would write file: /home/ubuntu/.kube/config
Preparing Juju
Would run: /usr/bin/snap install juju
Would create directory: /home/ubuntu/.local/share/juju
Would chown /home/ubuntu/.local to 1000:1000
Would run: sudo -u ubuntu juju show-controller concierge-lxd
Previous Juju controller 'concierge-lxd' found, skipping bootstrap
Would run: sudo -u ubuntu juju show-controller concierge-k8s
Previous Juju controller 'concierge-k8s' found, skipping bootstrap
ubuntu@concierge-dry-run:~/concierge$

benhoyt

Just leaving a few comments for now

internal/snapd/client_test.go

cmd/main.go

internal/concierge/manager.go

internal/concierge/manager_test.go

internal/system/interface.go

benhoyt · 2026-02-01T22:03:15Z

Per discussion, let's change the output format so you can basically copy 'n' paste into a shell script.

benhoyt

Looks good overall, and I really like the mostly-copy-n-pastable output. Several minor comments.

One other thing. I'm wondering if we can remove the /usr/bin and /usr/sbin prefix from lines like these:

/usr/bin/apt-get update
/usr/bin/apt-get install -y gnome-keyring
/usr/bin/apt-get install -y python3-pip
/usr/bin/apt-get install -y python3-venv
/usr/bin/snap install jhack --channel latest/stable
...
/usr/bin/chmod a+wr /var/snap/lxd/common/lxd/unix.socket
/usr/sbin/usermod -a -G lxd ben.hoyt@canonical.com
/usr/sbin/iptables -F FORWARD
/usr/sbin/iptables -P FORWARD ACCEPT
/usr/bin/snap install juju

I believe this comes from the fact that we do an exec.LookPath (basically which) and then store the full path. But it seems if we want to use LookPath to do a check, we could do that to check that the tool exists early, but then just throw away the path and exec the binary like chmod directly. (Or maybe we can just drop that altogether? I'm not sure.)

README.md

internal/concierge/manager.go

internal/config/config_format.go

internal/packages/dryrun_test.go

internal/system/dryrun.go

internal/system/dryrun_test.go

…he binary name.

internal/concierge/manager.go

dimaqq · 2026-02-05T05:35:18Z

From the chat: something wrong with 2x dryrun_test.go

tonyandrewmeyer · 2026-02-05T07:47:49Z

I need to fix some things here. I'll ping you when it's ready for review again (I'll try to get it done before Monday).

…those, and we have actual drynrun tests too, so this is really just pointless, so remove it.

Since we always succeed with commands in dry run mode, the technique where concierge will bootstrap until there is no error, relying on idempotent bootstrapping, doesn't really do what we want, since drynrun thinks that bootstrapping has already happened, but that is unlikely. so we output the command always in dry run mode. minor related change: instead of using 'which' to figure out if iptables is installed, use the native go functionality - because 'which' would always succeed.

tonyandrewmeyer · 2026-02-07T05:52:58Z

Output for prepare:

tameyer@tam-canoncial-1:~/code/concierge$ sudo go run ./main.go prepare -p dev --dry-run
[sudo] password for tameyer: 
apt-get update
apt-get install -y gnome-keyring
apt-get install -y python3-pip
apt-get install -y python3-venv
snap refresh snapcraft --channel latest/stable --classic
snap refresh jhack --channel latest/stable
snap connect jhack:dot-local-share-juju
snap refresh charmcraft --channel latest/stable --classic
snap refresh jq --channel latest/stable
snap refresh yq --channel latest/stable
snap refresh rockcraft --channel latest/stable --classic
snap refresh k8s --channel 1.32-classic/stable --classic
snap refresh lxd
lxd waitready --timeout 270
lxd init --minimal
lxc network set lxdbr0 ipv6.address none
chmod a+wr /var/snap/lxd/common/lxd/unix.socket
usermod -a -G lxd tameyer
iptables -F FORWARD
iptables -P FORWARD ACCEPT
snap install kubectl --channel stable --classic
k8s bootstrap
k8s status --wait-ready --timeout 270s
k8s set load-balancer.l2-mode=true
k8s set load-balancer.cidrs=10.43.45.0/28
k8s enable load-balancer
k8s enable local-storage
k8s enable network
k8s kubectl config view --raw
# Write file: /home/tameyer/.kube/config
snap refresh juju
mkdir -p /home/tameyer/.local/share/juju
chown -R 1000:1000 /home/tameyer/.local
sudo -u tameyer -g lxd juju bootstrap localhost concierge-lxd --verbose --model-default automatically-retry-hooks=false --model-default test-mode=true
sudo -u tameyer juju add-model -c concierge-lxd testing
sudo -u tameyer juju set-model-constraints -m concierge-lxd:testing arch=amd64
sudo -u tameyer juju bootstrap k8s concierge-k8s --verbose --model-default automatically-retry-hooks=false --model-default test-mode=true --bootstrap-constraints root-disk=2G
sudo -u tameyer juju add-model -c concierge-k8s testing
sudo -u tameyer juju set-model-constraints -m concierge-k8s:testing arch=amd64
tameyer@tam-canoncial-1:~/code/concierge$

Output for restore if prepare hasn't run:

tameyer@tam-canoncial-1:~/code/concierge$ sudo go run ./main.go restore --dry-run
time=2026-02-07T18:50:09.516+13:00 level=ERROR msg="concierge failed" error="failed to load previous runtime configuration: failed to read file: file '/home/tameyer/.cache/concierge/concierge.yaml' does not exist: stat /home/tameyer/.cache/concierge/concierge.yaml: no such file or directory"
exit status 1

Output for restore after prepare:

tameyer@tam-canoncial-1:~/code/concierge$ sudo go run ./main.go restore --dry-run
apt-get remove -y gnome-keyring
apt-get remove -y python3-pip
apt-get remove -y python3-venv
apt-get autoremove -y
snap remove jq --purge
snap remove rockcraft --purge
snap remove snapcraft --purge
snap remove yq --purge
snap remove charmcraft --purge
snap remove jhack --purge
snap remove lxd --purge
snap remove k8s --purge
snap remove kubectl --purge
rm -rf /home/tameyer/.kube
systemctl list-unit-files containerd.service
rm -rf /home/tameyer/.local/share/juju
snap remove juju --purge
tameyer@tam-canoncial-1:~/code/concierge$

tonyandrewmeyer · 2026-02-07T06:14:22Z

One change since you last reviewed: because Concierge does bootstrap by doing k8s status (etc) but in --dry-run all the commands succeed, it previously always assumed that K8s and Juju were already bootstrapped.

I changed it so that we always output the bootstrap commands, but that was the wrong call, I think. So now I've changed it so that users of system are responsible for saying "this command is read only" and then the dry-run worker can do the appropriate thing. It'd be great to talk this over with you in our 1-1 to see if this is the best approach or not.

internal/providers/k8s_test.go

internal/system/dryrun.go

benhoyt

Great, let's do it.

tonyandrewmeyer and others added 23 commits January 29, 2026 10:59

Address copilot review comments.

c503c9f

Initial plan

517e628

Merge pull request #3 from tonyandrewmeyer/copilot/sub-pr-2

e4732c9

feat: add dry-run option to prepare and restore (outstanding review comments)

Simplify README addition.

3cf2557

More minimal diff.

66fdfc3

Tweak punctuation.

8a78a81

Avoid having to define past and present tense.

f0189dd

Update README.md

286e243

Update README.md

6de765e

Fix name clash.

b329c5d

Sacrifice more human readable descriptions in favour of fewer custom …

3a9b8b6

…print lines. This also gives more similar 'you would execute this thing' type output, which I think is probably better.

Update internal/packages/snap_handler.go

5d72d39

Update README.md

9ce55e3

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/packages/dryrun_test.go

13aae96

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update cmd/main.go

a9bff3c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tests/dry-run-prepare/task.yaml

b5c3edd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tests/dry-run-restore/task.yaml

f73da87

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/system/dryrun.go

8c62bd2

tonyandrewmeyer requested a review from benhoyt January 31, 2026 06:07

tonyandrewmeyer commented Jan 31, 2026

View reviewed changes

internal/snapd/client_test.go Show resolved Hide resolved

Merge branch 'main' into feat-dry-run

bedc395

benhoyt reviewed Feb 1, 2026

View reviewed changes

tonyandrewmeyer requested a review from benhoyt February 3, 2026 04:10

benhoyt requested changes Feb 3, 2026

View reviewed changes

tonyandrewmeyer added 7 commits February 5, 2026 12:10

Do exact changes, per review suggestion.

61b01e0

Simplify dry-run explanation.

fe4888d

Dry run is a CLI arg, but isn't part of config.

4721a66

Keep checking if a command exists in the path, but run and log just t…

1832787

…he binary name.

restore should behave the same way with and without --dry-run

bec5014

Fix the way that dry-run is handled with restore.

3a0f694

Add integration tests that verify the load-saved-config behaviour.

1e79d0f

tonyandrewmeyer requested a review from benhoyt February 5, 2026 04:59

tonyandrewmeyer commented Feb 5, 2026

View reviewed changes

internal/concierge/manager.go Outdated Show resolved Hide resolved

tonyandrewmeyer marked this pull request as draft February 5, 2026 07:47

tonyandrewmeyer added 2 commits February 7, 2026 18:42

This test is really just testing the packages, and we have tests for …

15af2b8

…those, and we have actual drynrun tests too, so this is really just pointless, so remove it.

tonyandrewmeyer added 2 commits February 7, 2026 18:53

Update internal/concierge/manager.go

81e1c80

Change approach to run commands with an optional read-only flag.

cc887bf

tonyandrewmeyer marked this pull request as ready for review February 7, 2026 06:24

benhoyt reviewed Feb 8, 2026

View reviewed changes

internal/providers/k8s_test.go Outdated Show resolved Hide resolved

internal/system/dryrun.go Outdated Show resolved Hide resolved

Use the readonly flag for which iptables as well.

e39051d

tonyandrewmeyer requested a review from benhoyt February 15, 2026 20:31

benhoyt approved these changes Feb 15, 2026

View reviewed changes

tonyandrewmeyer closed this Feb 16, 2026

tonyandrewmeyer reopened this Feb 16, 2026

Merge branch 'main' into feat-dry-run

6b11a49

tonyandrewmeyer merged commit bebf251 into canonical:main Feb 17, 2026
56 of 60 checks passed

tonyandrewmeyer deleted the feat-dry-run branch February 17, 2026 10:02

Conversation

tonyandrewmeyer commented Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tonyandrewmeyer commented Jan 31, 2026

Uh oh!

tonyandrewmeyer commented Jan 31, 2026

Uh oh!

benhoyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benhoyt commented Feb 1, 2026

Uh oh!

benhoyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimaqq commented Feb 5, 2026

Uh oh!

tonyandrewmeyer commented Feb 5, 2026

Uh oh!

tonyandrewmeyer commented Feb 7, 2026

Uh oh!

tonyandrewmeyer commented Feb 7, 2026

Uh oh!

Uh oh!

Uh oh!

benhoyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tonyandrewmeyer commented Jan 31, 2026 •

edited

Loading