feat: Enable specifying a validation-set sequence number by jocado · Pull Request #292 · canonical/ubuntu-image

jocado · 2026-04-08T11:23:15Z

Add support for specifying the sequence number of given validation-set, but without having to lock the sequence number in the the model itself.

The new --sequence option is used to allow specifying a sequence number for a named valediction-set. This is similar to the --revision option allows specifying revisions of for named snaps.

--sequence=my-validatin-set:123 --sequence=my-other-validatin-set:456

If you specify the sequence number for the validation-set in the model assertion itself, then the device is locked to that for the life of the model on that device. This allows setting an install-time sequence number, and still allowing the sequence to changed later on a given device based on the specific model in question.

Add support for specifying the sequence number of given validation-set, but without having to lock the sequeunce number in the the model itself.

jocado · 2026-04-08T12:36:13Z

@upils Apologies for tagging you, but I wasn't sure of the PR workflow in this repo, and how best to get a review, so I'm asking for some guidance as someone who's been involved in various PRs here 🙂

Are you able to help review, or otherwise advise me of any process I should follow ?

Thanks!

mwhudson

Bearing in mind that I am reviewing this code through a telescope and not really understanding the code around these changes at all, it looks OK enough to me. I assume you've tested that they help your use case and they do not look like they will disrupt any existing users.

I do have just a couple of questions though. They are not really blockers.

mwhudson · 2026-04-20T19:09:26Z

+			}
+			modelValidationSets[validationSet.Name] = validationSet
+		}
+	}


This feels a little weird in that is checking something about the model but only when you pass a particular flag. It is valid in general to have a model with two validation sets with the same name?

Thanks for taking a look at this 👍

This is because validation-sets in models are a list of dicts of the following form:

{ "account-id": "abcdefg.....", "name": "name-of-a-validation-set", "mode": "enforce" }

The name of the validation set is only unique in a specific account [ as far as I understand ], so this is trying to protect against the fact that the name could be ambiguous if there was another validation set from a different account with the same name added to the model assertions.
It's extremely unlikely I would say, but seems possible at least.
This really just allows just using the validation-set name in the args without the account prefixed to make it unique [ which would be a bit clunky to use ], while protecting against the unlikely name clash.

I hope that makes some sense.

@jocado If there were an ambiguous name (2 validation sets from different accounts, with the same name), in this implementation ubuntu-image would error out. But then what would be the next step for the user? Recreate another validation set with a different name for one of the account to avoid the collision?

Another solution could be to accept a short form for --sequence, like <name>:<sequence> but also a long form <account-id>/<name>:<sequence> to avoid the ambiguity when a collision happens. So the "clunkiness" would be limited to rare ambiguous cases. WDYT?

mwhudson · 2026-04-20T19:10:58Z

+			return nil, fmt.Errorf("error dealing with validation-set sequence %s: validation-set not present in model assertion", validationSetName)
+		}
+
+		fmt.Printf("WARNING: sequence %d for validation-set %s may not be the latest available sequence!\n", sequence, validationSetName)


Printing this warning every time the feature is used seems a little user hostile because there is no action the user can take to stop it. Is that a fundamental limitation or something that could be fixed with enough effort?

It's not a limitation as such, just a fact that if the end user has specified a sequence number manually, there's chance it's not the latest available sequence for that validation set. It's the same as manually specifying a revision for a snap, it may not be the latest one.

But, I would be happy to drop this if you think it's a bit over the top ? If someone is specificity a particular sequence, they are probably already aware if it's the latest or not, or that it may not be the latest in the future [ if it's statically defined for a time in a pipeline or something ].

As you pointed out, your approach here is consistent with how the existing code deal with snap with revisions, so I think the warning is fine here to make sure the user is not blindly using an old sequence without thinking out it.

@mwhudson I agree that this is not great that the user cannot do anything about it. I do not think this is a fundamental limitation, we could imagine checking the store for newer revisions/sequence and not display the warning if the latest one is used. But this is likely out of scope of this PR and something we are unlikely to spend the required effort on given the state of the project.

Yeah I have a thing about warnings that the user cannot avoid but I agree it's not worth doing anything about in the context of this PR!

Copilot

Pull request overview

Adds support for overriding validation-set sequence numbers at image build time via a new --sequence option, without pinning sequences into the model assertion.

Changes:

Introduces --sequence flag on the snap command (map[name]sequence) to override validation-set sequences.
Extends seed manifest generation to add allowed validation-set entries based on --sequence.
Adds unit tests and model-assertion testdata covering the new behavior and error handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
internal/statemachine/testdata/modelAssertionValidationSets	Adds a model assertion fixture containing multiple validation-sets for sequence override tests.
internal/statemachine/snap_test.go	Adds tests validating sequence overrides are passed into the seed manifest and missing-set errors are surfaced.
internal/statemachine/snap_states.go	Populates seed manifest with allowed validation-set sequence overrides (and adds snapd asserts dependency).
internal/commands/snap.go	Adds the `--sequence` CLI option to SnapOpts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-22T13:53:33Z

@@ -69,9 +70,26 @@ func (stateMachine *StateMachine) prepareImage() error {

 // imageOptsSeedManifest sets up the pre-provided manifest if revisions are passed


The function comment is now inaccurate: imageOptsSeedManifest no longer only handles snap revision overrides; it also adds validation-set sequence overrides. Update the comment to reflect both behaviors to avoid misleading future changes/tests.

Suggested change

// imageOptsSeedManifest sets up the pre-provided manifest if revisions are passed

// imageOptsSeedManifest sets up the pre-provided manifest when snap revision

// overrides or validation-set sequence overrides are passed.

Copilot · 2026-04-22T13:53:34Z

+	expectedValidationSets := calledOpts.SeedManifest.AllowedValidationSets()
+	if len(expectedValidationSets) != 1 {
+		t.Fatalf("expected 1 validation-set override, got %d", len(expectedValidationSets))
+	}
+
+	expected := expectedValidationSets[0]
+	if expected.AccountID != "CA5GLZgNQWPhspDQK63Er46Uxz2SO7ez" {
+		t.Fatalf("expected account-id %q, got %q", "CA5GLZgNQWPhspDQK63Er46Uxz2SO7ez", expected.AccountID)
+	}
+	if expected.Name != "test-validation-set" {
+		t.Fatalf("expected validation-set name %q, got %q", "test-validation-set", expected.Name)
+	}
+	if expected.Sequence != 7 {
+		t.Fatalf("expected sequence %d, got %d", 7, expected.Sequence)
+	}
+	if expected.Pinned {


Variable name is misleading here: expectedValidationSets holds the value returned from the code under test (i.e., the actual/got validation-set overrides), not an expected value. Renaming to something like validationSets/gotValidationSets will make the assertions easier to read.

Suggested change

expectedValidationSets := calledOpts.SeedManifest.AllowedValidationSets()

if len(expectedValidationSets) != 1 {

t.Fatalf("expected 1 validation-set override, got %d", len(expectedValidationSets))

}

expected := expectedValidationSets[0]

if expected.AccountID != "CA5GLZgNQWPhspDQK63Er46Uxz2SO7ez" {

t.Fatalf("expected account-id %q, got %q", "CA5GLZgNQWPhspDQK63Er46Uxz2SO7ez", expected.AccountID)

}

if expected.Name != "test-validation-set" {

t.Fatalf("expected validation-set name %q, got %q", "test-validation-set", expected.Name)

}

if expected.Sequence != 7 {

t.Fatalf("expected sequence %d, got %d", 7, expected.Sequence)

}

if expected.Pinned {

validationSets := calledOpts.SeedManifest.AllowedValidationSets()

if len(validationSets) != 1 {

t.Fatalf("expected 1 validation-set override, got %d", len(validationSets))

}

validationSet := validationSets[0]

if validationSet.AccountID != "CA5GLZgNQWPhspDQK63Er46Uxz2SO7ez" {

t.Fatalf("expected account-id %q, got %q", "CA5GLZgNQWPhspDQK63Er46Uxz2SO7ez", validationSet.AccountID)

}

if validationSet.Name != "test-validation-set" {

t.Fatalf("expected validation-set name %q, got %q", "test-validation-set", validationSet.Name)

}

if validationSet.Sequence != 7 {

t.Fatalf("expected sequence %d, got %d", 7, validationSet.Sequence)

}

if validationSet.Pinned {

upils

Thanks @jocado for your contribution! Overall it looks great. See my comments on other discussions and the Copilot review.

Also would you mind testing the case with ambiguous names? (I know it could change based on the other discussion).

feat: Enable specifying a validation-set sequence number

17c0744

Add support for specifying the sequence number of given validation-set, but without having to lock the sequeunce number in the the model itself.

mwhudson reviewed Apr 20, 2026

View reviewed changes

upils requested a review from Copilot April 22, 2026 13:46

Copilot started reviewing on behalf of upils April 22, 2026 13:47 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

upils requested changes Apr 23, 2026

View reviewed changes

		@@ -69,9 +70,26 @@ func (stateMachine *StateMachine) prepareImage() error {

		// imageOptsSeedManifest sets up the pre-provided manifest if revisions are passed

	// imageOptsSeedManifest sets up the pre-provided manifest if revisions are passed
	// imageOptsSeedManifest sets up the pre-provided manifest when snap revision
	// overrides or validation-set sequence overrides are passed.

Conversation

jocado commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jocado commented Apr 8, 2026

Uh oh!

mwhudson left a comment

Choose a reason for hiding this comment

Uh oh!

mwhudson Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

jocado Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

upils Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

mwhudson Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

jocado Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

upils Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

mwhudson Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jocado commented Apr 8, 2026 •

edited

Loading

jocado Apr 20, 2026 •

edited

Loading