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

fix: deprecate "chisel-v1" format #150

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

letFunny
Copy link
Collaborator

@letFunny letFunny commented Jul 29, 2024

This commit depreciates support for the "chisel-v1" format in chisel. Notably the following fields/values are no longer supported:

- The "chisel-v1" value for top-level field "format". Use "v1"
  instead.
- The "<archive>.v1-public-keys" field. Use "<archive>.public-keys"
  instead.
- The top-level "v1-public-keys" field. Use "public-keys" instead.

BREAKING CHANGE: New versions of chisel which includes this commit will
no longer support the "chisel-v1" format in chisel-releases. Either
update the "chisel.yaml" file in chisel-releases or use a lower
version which does not have this commit.


  • Have you signed the CLA?

@letFunny letFunny added the Simple Nice for a quick look on a minute or two label Jul 29, 2024
This commit depreciates support for the "chisel-v1" format in chisel.
Notably the following fields/values are no longer supported:

    - The "chisel-v1" value for top-level field "format". Use "v1"
      instead.
    - The "<archive>.v1-public-keys" field. Use "<archive>.public-keys"
      instead.
    - The top-level "v1-public-keys" field. Use "public-keys" instead.

BREAKING CHANGE: New versions of chisel which includes this commit will
    no longer support the "chisel-v1" format in chisel-releases. Either
    update the "chisel.yaml" file in chisel-releases or use a lower
    version which does not have this commit.
@letFunny letFunny added the Priority Look at me first label Oct 3, 2024
@letFunny letFunny requested a review from niemeyer October 3, 2024 10:36
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.

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.

Looks good, thank you.

@niemeyer niemeyer merged commit 944ee7c into canonical:main Oct 7, 2024
14 checks passed
@letFunny letFunny deleted the deprecate-chisel-v1 branch October 17, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first Simple Nice for a quick look on a minute or two
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants