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.

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

feat: make suites mandatory in archive config #161

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

HadrienPatte
Copy link
Contributor

  • Have you signed the CLA?

This PR makes the suites attribute of archives in the chisel.yaml mandatory. All other attributes are already mandatory (version, components and keys).
There was an old fallback logic which would set suites to the release adjective if left unset, but this logic was only configured for 4 ubuntu releases, one of which (bionic) is no longer supported by chisel, and all other three have their suites properly configured in their chisel.yaml already, so this piece of logic never triggers (focal, jammy, kinetic).

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Please note that some other packages (main and slicer) have tests without the archives.<archive>.suites field. We need to update those too.

I can't trigger the CI, but the unit tests are failing actually (ran locally):

$ go test ./... -count=
1
?       github.com/canonical/chisel/cmd [no test files]
?       github.com/canonical/chisel/internal/archive/testarchive        [no test files]

----------------------------------------------------------------------
FAIL: cmd_info_test.go:209: ChiselSuite.TestInfoCommand

Summary: A single slice inspection
cmd_info_test.go:230:
    c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"chisel.yaml: archive \"ubuntu\" missing suites field"} ("chisel.yaml: archive \"ubuntu\" missing suites field")

OOPS: 2 passed, 1 FAILED
--- FAIL: Test (0.01s)
FAIL
FAIL    github.com/canonical/chisel/cmd/chisel  0.028s
ok      github.com/canonical/chisel/internal/archive    0.193s
ok      github.com/canonical/chisel/internal/cache      0.014s
ok      github.com/canonical/chisel/internal/control    0.006s
?       github.com/canonical/chisel/internal/deb/chrorder       [no test files]
ok      github.com/canonical/chisel/internal/deb        0.067s
ok      github.com/canonical/chisel/internal/fsutil     0.033s
ok      github.com/canonical/chisel/internal/jsonwall   0.007s
ok      github.com/canonical/chisel/internal/manifest   0.013s
ok      github.com/canonical/chisel/internal/pgputil    0.024s
ok      github.com/canonical/chisel/internal/scripts    0.041s
ok      github.com/canonical/chisel/internal/setup      17.924s

----------------------------------------------------------------------
FAIL: slicer_test.go:1090: S.TestRun

Summary: Basic slicing
slicer_test.go:1092:
    // Run tests for format chisel-v1.
    runSlicerTests(c, slicerTests)
slicer_test.go:1136:
    c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"chisel.yaml: archive \"ubuntu\" missing suites field"} ("chisel.yaml: archive \"ubuntu\" missing suites field")

OOPS: 2 passed, 1 FAILED
--- FAIL: Test (0.01s)
FAIL
FAIL    github.com/canonical/chisel/internal/slicer     0.100s
ok      github.com/canonical/chisel/internal/strdist    0.007s
ok      github.com/canonical/chisel/internal/testutil   12.272s
FAIL

internal/setup/setup_test.go Show resolved Hide resolved
Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

Looks good to me. We need to discuss if we want to remove the defaults or configure them properly, probably the former.

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

This looks reasonable, thanks for the suggestion. We need to discuss the possible removal of the version field as well, which was used for assuming details before. But we'll do it separately.

@niemeyer niemeyer merged commit 022d771 into canonical:main Oct 14, 2024
13 of 14 checks passed
@HadrienPatte HadrienPatte deleted the adjectives branch October 14, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants