feat: add support for chisel release format "v1" #115
Conversation
This commits adds support for "v1" which changes the "v1-public-keys" field into "public-keys". The "chisel-v1" format is still supported.
internal/setup/releaseV1.go
Outdated
| "github.com/canonical/chisel/internal/pgputil" | ||
| ) | ||
|
|
||
| func parseReleaseV1(baseDir, filePath string, data []byte) (*Release, error) { |
There was a problem hiding this comment.
[For reviewer]: Diff against chisel-v1 https://www.diffchecker.com/nYQFzN77/ .
internal/setup/releaseChiselV1.go
Outdated
| "github.com/canonical/chisel/internal/pgputil" | ||
| ) | ||
|
|
||
| func parseReleaseChiselV1(baseDir, filePath string, data []byte) (*Release, error) { |
There was a problem hiding this comment.
[For reviewer]: The code was moved from setup.go and the only change was moving the types inside of the function to prevent name clashes with v1. Proof: https://www.diffchecker.com/zg6qjOmM/ .
internal/setup/setup.go
Outdated
| Components []string `yaml:"components"` | ||
| Default bool `yaml:"default"` | ||
| PubKeys []string `yaml:"v1-public-keys"` | ||
| switch yamlVar.Format { |
There was a problem hiding this comment.
[For reviewer]: I have chosen to add the two formats separately because that is the more flexible option. If we end up having to support chisel-v1 longer than expected or we need to do some change in v1, it would be far easier to do it if they are not entangled. Additionally, we can remove and add formats easily with this structure (e.g. removing chisel-v1 is as easy as deleting the file).
The implementation happens to be similar but they represent different format so, in my opinion, we should not strive for DRY here.
internal/setup/releaseChiselV1.go
Outdated
| type yamlRelease struct { | ||
| Format string `yaml:"format"` | ||
| Archives map[string]yamlArchive `yaml:"archives"` | ||
| PubKeys map[string]yamlPubKey `yaml:"v1-public-keys"` |
There was a problem hiding this comment.
It doesn't feel like we need all of this logic when the only thing changing is the name of this field. How about adding the two fields to the same struct, and if the format is the old one, just put it in the new field? Everything else could be the same, I guess?
There was a problem hiding this comment.
I had the same discussion with Rafid on Friday, which is why I added a comment to try to explain it. I see your point about the code being much sorter that way (even with some extra logic for error reporting). However, I think it couples both format parsers when they should be independent, for example if we have to change v1 or support a third format at the same time. It just happens that the implementation is similar even though they represent different things.
There was a problem hiding this comment.
I will make your proposed changes if you disagree, just wanted to make sure I got the rationale across correctly.
There was a problem hiding this comment.
A few points:
- A couple of lines is much better to maintain than hundreds and multiple files with repeated code
- We cannot break v1 after it's released, so we won't break chisel-v1 either because the only delta is the two lines
- chisel-v1 will be completely gone in a few weeks because chisel-releases will be updated
There was a problem hiding this comment.
I see your points, but I still think the flexibility and separation of concerns is still worth it. Especially if there are unforeseen changes that make it necessary to not drop "chisel-v1" in a few weeks. That said, I disagree and commit, I have made the changes to the PR.
This commits adds support for "v1" which changes the "v1-public-keys" field into "public-keys". The "chisel-v1" format is still supported.