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

fix: deprecate "chisel-v1" format #150

Merged
merged 4 commits into from
Oct 7, 2024
Merged

fix: deprecate "chisel-v1" format #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
322d890
fix: deprecate "chisel-v1" format (#16)
rebornplusplus Jul 29, 2024
c3213c7
Merge branch 'main' into deprecate-chisel-v1
letFunny Aug 6, 2024
21bf603
Merge branch 'main' into deprecate-chisel-v1
letFunny Oct 3, 2024
5bc7031
new usages of chisel-v1
letFunny Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmd/chisel/cmd_info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,13 @@ var infoTests = []infoTest{{
var testKey = testutil.PGPKeys["key1"]

var defaultChiselYaml = `
format: chisel-v1
format: v1
archives:
ubuntu:
version: 22.04
components: [main, universe]
v1-public-keys: [test-key]
v1-public-keys:
public-keys: [test-key]
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t")
Expand Down
21 changes: 2 additions & 19 deletions internal/setup/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,6 @@ type yamlRelease struct {
Format string `yaml:"format"`
Archives map[string]yamlArchive `yaml:"archives"`
PubKeys map[string]yamlPubKey `yaml:"public-keys"`
// V1PubKeys is used for compatibility with format "chisel-v1".
V1PubKeys map[string]yamlPubKey `yaml:"v1-public-keys"`
}

type yamlArchive struct {
Expand All @@ -378,8 +376,6 @@ type yamlArchive struct {
Components []string `yaml:"components"`
Default bool `yaml:"default"`
PubKeys []string `yaml:"public-keys"`
// V1PubKeys is used for compatibility with format "chisel-v1".
V1PubKeys []string `yaml:"v1-public-keys"`
}

type yamlPackage struct {
Expand Down Expand Up @@ -504,18 +500,9 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
if err != nil {
return nil, fmt.Errorf("%s: cannot parse release definition: %v", fileName, err)
}
if yamlVar.Format != "chisel-v1" && yamlVar.Format != "v1" {
if yamlVar.Format != "v1" {
return nil, fmt.Errorf("%s: unknown format %q", fileName, yamlVar.Format)
}
// If format is "chisel-v1" we have to translate from the yaml key "v1-public-keys" to
// "public-keys".
if yamlVar.Format == "chisel-v1" {
yamlVar.PubKeys = yamlVar.V1PubKeys
for name, details := range yamlVar.Archives {
details.PubKeys = details.V1PubKeys
yamlVar.Archives[name] = details
}
}
if len(yamlVar.Archives) == 0 {
return nil, fmt.Errorf("%s: no archives defined", fileName)
}
Expand Down Expand Up @@ -556,11 +543,7 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
release.DefaultArchive = archiveName
}
if len(details.PubKeys) == 0 {
if yamlVar.Format == "chisel-v1" {
return nil, fmt.Errorf("%s: archive %q missing v1-public-keys field", fileName, archiveName)
} else {
return nil, fmt.Errorf("%s: archive %q missing public-keys field", fileName, archiveName)
}
return nil, fmt.Errorf("%s: archive %q missing public-keys field", fileName, archiveName)
}
var archiveKeys []*packet.PublicKey
for _, keyName := range details.PubKeys {
Expand Down
98 changes: 48 additions & 50 deletions internal/setup/setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ var setupTests = []setupTest{{
summary: "Missing archives",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
`,
},
relerror: `chisel.yaml: no archives defined`,
Expand All @@ -56,14 +56,14 @@ var setupTests = []setupTest{{
summary: "Archive with multiple suites",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
ubuntu:
version: 22.04
components: [main, other]
suites: [jammy, jammy-security]
v1-public-keys: [test-key]
v1-public-keys:
public-keys: [test-key]
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
Expand Down Expand Up @@ -769,20 +769,20 @@ var setupTests = []setupTest{{
summary: "Multiple archives",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
components: [main, universe]
suites: [jammy]
default: true
v1-public-keys: [test-key]
public-keys: [test-key]
bar:
version: 22.04
components: [universe]
suites: [jammy-updates]
v1-public-keys: [test-key]
v1-public-keys:
public-keys: [test-key]
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
Expand Down Expand Up @@ -823,16 +823,16 @@ var setupTests = []setupTest{{
summary: "Extra fields in YAML are ignored (necessary for forward compatibility)",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
ubuntu:
version: 22.04
components: [main, other]
suites: [jammy, jammy-security]
v1-public-keys: [test-key]
public-keys: [test-key]
madeUpKey1: whatever
madeUpKey2: whatever
v1-public-keys:
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
Expand Down Expand Up @@ -881,20 +881,20 @@ var setupTests = []setupTest{{
summary: "Archives with public keys",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
components: [main, universe]
suites: [jammy]
v1-public-keys: [extra-key]
public-keys: [extra-key]
default: true
bar:
version: 22.04
components: [universe]
suites: [jammy-updates]
v1-public-keys: [test-key, extra-key]
v1-public-keys:
public-keys: [test-key, extra-key]
public-keys:
extra-key:
id: ` + extraTestKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(extraTestKey.PubKeyArmor, "\t\t\t\t\t\t") + `
Expand Down Expand Up @@ -938,7 +938,7 @@ var setupTests = []setupTest{{
summary: "Archive without public keys",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
Expand All @@ -947,18 +947,18 @@ var setupTests = []setupTest{{
default: true
`,
},
relerror: `chisel.yaml: archive "foo" missing v1-public-keys field`,
relerror: `chisel.yaml: archive "foo" missing public-keys field`,
}, {
summary: "Unknown public key",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
components: [main, universe]
suites: [jammy]
v1-public-keys: [extra-key]
public-keys: [extra-key]
default: true
`,
"slices/mydir/mypkg.yaml": `
Expand All @@ -970,15 +970,15 @@ var setupTests = []setupTest{{
summary: "Invalid public key",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
components: [main, universe]
suites: [jammy]
v1-public-keys: [extra-key]
public-keys: [extra-key]
default: true
v1-public-keys:
public-keys:
extra-key:
id: foo
armor: |
Expand All @@ -998,15 +998,15 @@ var setupTests = []setupTest{{
summary: "Mismatched public key ID",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
components: [main, universe]
suites: [jammy]
v1-public-keys: [extra-key]
public-keys: [extra-key]
default: true
v1-public-keys:
public-keys:
extra-key:
id: ` + extraTestKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
Expand Down Expand Up @@ -1562,44 +1562,42 @@ var setupTests = []setupTest{{
`,
},
relerror: `slice mypkg_myslice path /path/\*\* has invalid generate options`,
}, {
summary: "chisel-v1 is deprecated",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
archives:
foo:
version: 22.04
components: [main, universe]
suites: [jammy]
v1-public-keys: [test-key]
default: true
v1-public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
`,
},
relerror: `chisel.yaml: unknown format "chisel-v1"`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[Question for reviewer]: The only question is whether we should introduce a better error message for chisel-v1 that says something like "consider an update if available" just like we do for invalid generate: values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the conversation we had, to introduce such a message we need to do that at the very top layer, in the user interface itself (CLI, or GUI) as otherwise we cannot tell the context in which the library/package is being used. Even there, sometimes it may be misleading as the tool may be leveraged in servers and the error reported in a way that is impossible for the receiver of the message to act on the suggestion. For that kind of reason, it is usually better to report the error with a good error message which allows the reader to grasp what is going on by themselves, which seems like the case here already.

}}

var defaultChiselYaml = `
format: chisel-v1
format: v1
archives:
ubuntu:
version: 22.04
components: [main, universe]
v1-public-keys: [test-key]
v1-public-keys:
public-keys: [test-key]
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
`

func (s *S) TestParseRelease(c *C) {
// Run tests for format chisel-v1.
runParseReleaseTests(c, setupTests)

// Run tests for format v1.
v1SetupTests := make([]setupTest, len(setupTests))
for i, t := range setupTests {
t.relerror = strings.Replace(t.relerror, "chisel-v1", "v1", -1)
t.relerror = strings.Replace(t.relerror, "v1-public-keys", "public-keys", -1)
m := map[string]string{}
for k, v := range t.input {
v = strings.Replace(v, "chisel-v1", "v1", -1)
v = strings.Replace(v, "v1-public-keys", "public-keys", -1)
m[k] = v
}
t.input = m
v1SetupTests[i] = t
}
runParseReleaseTests(c, v1SetupTests)
}

func runParseReleaseTests(c *C, tests []setupTest) {
for _, test := range tests {
for _, test := range setupTests {
c.Logf("Summary: %s", test.summary)

if _, ok := test.input["chisel.yaml"]; !ok {
Expand Down
37 changes: 8 additions & 29 deletions internal/slicer/slicer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,18 +773,18 @@ var slicerTests = []slicerTest{{
slices: []setup.SliceKey{{"test-package", "myslice"}},
release: map[string]string{
"chisel.yaml": `
format: chisel-v1
format: v1
archives:
foo:
version: 22.04
components: [main, universe]
default: true
v1-public-keys: [test-key]
public-keys: [test-key]
bar:
version: 22.04
components: [main]
v1-public-keys: [test-key]
v1-public-keys:
public-keys: [test-key]
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
Expand Down Expand Up @@ -1197,41 +1197,20 @@ var slicerTests = []slicerTest{{
}}

var defaultChiselYaml = `
format: chisel-v1
format: v1
archives:
ubuntu:
version: 22.04
components: [main, universe]
v1-public-keys: [test-key]
v1-public-keys:
public-keys: [test-key]
public-keys:
test-key:
id: ` + testKey.ID + `
armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
`

func (s *S) TestRun(c *C) {
// Run tests for format chisel-v1.
runSlicerTests(c, slicerTests)

// Run tests for format v1.
v1SlicerTests := make([]slicerTest, len(slicerTests))
for i, t := range slicerTests {
t.error = strings.Replace(t.error, "chisel-v1", "v1", -1)
t.error = strings.Replace(t.error, "v1-public-keys", "public-keys", -1)
m := map[string]string{}
for k, v := range t.release {
v = strings.Replace(v, "chisel-v1", "v1", -1)
v = strings.Replace(v, "v1-public-keys", "public-keys", -1)
m[k] = v
}
t.release = m
v1SlicerTests[i] = t
}
runSlicerTests(c, v1SlicerTests)
}

func runSlicerTests(c *C, tests []slicerTest) {
for _, test := range tests {
for _, test := range slicerTests {
for _, slices := range testutil.Permutations(test.slices) {
c.Logf("Summary: %s", test.summary)

Expand Down
Loading