-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add ability to append Container Linux snippets #22
Conversation
glide.yaml
Outdated
@@ -49,9 +49,11 @@ import: | |||
subpackages: | |||
- semver | |||
- package: github.com/coreos/ignition | |||
version: v0.19.0 | |||
version: 28aea78861d53a9b3a9c2b5cf51305e00f36fa14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not vendor a release of Ignition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I required coreos/ignition#501, otherwise, we have to copy paste the Append code out of Ignition, because none of the packages had appropriate types. Once there is a release, definitely planning to bump to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. There should be v0.21.0 this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely won't be ready until after then. I'm kinda nudging it along in spare time. I'll plan to bump to that version.
platform := d.Get("platform").(string) | ||
snippetsIface := d.Get("snippets").([]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with terraform libraries, but why not assert this to be a []string
? It feels like if the input doesn't match that it would be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try that originally. The underlying data Terraform Get gives here is actually a slice of interfaces so the assertion to []string
fails. I'll look into this again, the rampant use of assertions is saddening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, Go doesn't let you assert a []interface{}
to a []string
, but it does let you iterate over a []interface
and assert each thing individually to a string
. What a great type system.
I didn't read the code, but does this continually shove appended configs as dataurls into the parent config? |
What do you mean by "continually"? Describing it that way makes me think there is a misunderstanding. The tool combines one or more Container Linux configs into a single config. If you change the input configs, yes you get a new output config. All manner of things can be done with that config (e.g. write it to a local file, upload it to a cloud provider, post it somewhere, etc). No, not dataurls. The test case makes it a little clearer - it appends (like a merge, but not quite a merge) configs as if they had been written as one document. |
@crawford no, this calls the This happens here: https://github.com/coreos/terraform-provider-ct/pull/22/files#diff-ce3114c3cfc8a252dcbdb551d95e066bR86. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
* Allow users to supply a list of Container Linux Configs that should be appended. This means base configs can be additively extended with custom bits of Container Linux configs
tldr; I'm fully behind this feature now and vouch for it. Caveats about cutting the v0.3.0 release. I had some initial problems in practical usage (turns out unrelated). Since this Terraform provider isn't an official provider, it is installed in a global location (~/.terraformrc, rather than each module defining the version it would like). Using a build of We need to be careful. Essentially,
Action:
I've created a protected v0.2.x branch to continue supporting Ignition v2.0.0 series clusters. |
I'd like to cut the v0.3.0 release with this feature, bearing in mind the above caveats. |
Friendly ping :) Any news here? An up-to-date v0.3.0 with this in would be great - to have both Ignition 2.1 and snippets. |
Should be ready for merge. As long as release notes describe the different release series and correspond to a particular Ignition version - upgrading I no longer have merge ability though. cc @sdemos |
I'll release this at some point later today. |
Sorry about that, I started to do the release then something came up. It's released now though! https://github.com/coreos/terraform-provider-ct/releases/tag/v0.3.0 Also, @schu for what it's worth, snippets are also cherry-picked into the v0.2.1 release. It still outputs Ignition 2.0, though, so definitely worth updating for the newer spec if possible. |
* Add support for Fedora CoreOS `snippets` * Add a Fedora CoreOS example to the README * Move example to examples for project consistency Related: #22
* Add support for Fedora CoreOS `snippets` * Add a Fedora CoreOS example to the README * Move example to examples for project consistency Related: #22
Allow users to supply a list of Container Linux Configs that should be appended. This means base
configs can be additively extended with custom snippets of Container Linux configs. This is a long-awaited feature to unlock end-user customizations.
Testing
I'm putting this up for comment, but still working on validating the usage in real-world scenarios.
rel: https://github.com/coreos/matchbox/issues/622