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

spec: unify resource fetching type #953

Merged
merged 2 commits into from
Apr 15, 2020
Merged

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Mar 30, 2020

Dedupe the "remote resource" type from the spec. This came up too during
review of the new HTTP headers. They all had more or less the same
fields in common ("source", "verification", "compression", and more
recently "httpHeaders").

One noticeable spec level change from this is that some places now
support compression where they didn't before. Might seem odd to support
compression for e.g. certificate authorities, though it does make the
spec more consistent throughout.

It's a nice cleanup, though my goal with doing this is the ability to
inspect more easily whether a config contains any resources which
require some form of networking.

@jlebon
Copy link
Member Author

jlebon commented Mar 30, 2020

Marking WIP for now. Haven't tested this much yet, nor run blackbox testing. There's also some subtle changes in validation since in some places source wasn't required and others it was. I think I caught those, but need to double-check.

One noticeable spec level change from this is that some places now
support compression where they didn't before. Might seem odd to support
compression for e.g. certificate authorities, though it does make the
spec more consistent throughout.

Still, if we prefer to keep the spec file unchanged for now, we can split this into Resource and CompressibleResource.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

This LGTM generally; thanks for doing it! I don't see an obvious downside to making all resources compressible.

Tests to add:

  • Compression tests for certificateAuthorities, replace, and merge
  • Negative tests for certificateAuthorities, replace, and merge without a source specified

doc/configuration-v3_1-experimental.md Outdated Show resolved Hide resolved
config/v3_1_experimental/types/ignition.go Show resolved Hide resolved
config/v3_1_experimental/types/resource.go Show resolved Hide resolved
internal/resource/http.go Show resolved Hide resolved
@bgilbert bgilbert added this to the Spec 3.1.0 milestone Apr 7, 2020
bgilbert and others added 2 commits April 9, 2020 14:37
Dedupe the "remote resource" type from the spec. This came up too during
review of the new HTTP headers. They all had more or less the same
fields in common ("source", "verification", "compression", and more
recently "httpHeaders").

One noticeable spec level change from this is that some places now
support compression where they didn't before. Might seem odd to support
compression for e.g. certificate authorities, though it does make the
spec more consistent throughout.

Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Also verify hashes, to ensure that Ignition's decompress-before-verify
semantics are maintained (coreos#961).
@bgilbert bgilbert changed the title WIP: spec: unify resource fetching type spec: unify resource fetching type Apr 9, 2020
@bgilbert bgilbert marked this pull request as ready for review April 9, 2020 18:50
@bgilbert
Copy link
Contributor

bgilbert commented Apr 9, 2020

Okay, this should be ready for review. @jlebon, please take a look, and it'd be good to have a fresh reviewer also. @lucab?

@bgilbert bgilbert requested a review from lucab April 14, 2020 00:10
Copy link
Member Author

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for picking this up and pushing it through, LGTM! (Looks like I can't actually approve it since it's my own PR.)

config/v3_1_experimental/types/ignition.go Show resolved Hide resolved
@lucab
Copy link
Contributor

lucab commented Apr 15, 2020

Code LGTM and the PR is stamped.
I do have a minor question on the public type change and semver, but I do think it's ok under the rules of "experimental".

@bgilbert bgilbert merged commit 6708d9a into coreos:master Apr 15, 2020
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.

None yet

3 participants