-
Notifications
You must be signed in to change notification settings - Fork 1.5k
pkg/types/config: Break into pkg/validate and pkg/tfvars #412
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
pkg/types/config: Break into pkg/validate and pkg/tfvars #412
Conversation
5882b2e to
4d47856
Compare
4d47856 to
a0a1dbc
Compare
|
Can we not create a new package that has types. we already have pkg/types. maybe just a new file. |
We already have types all over: $ git describe
v0.1.0-30-ge4c8898
$ for DIR in pkg/*; do echo -n "${DIR} "; git grep '^type ' "${DIR}" | wc -l; done
pkg/asset 72
pkg/destroy 5
pkg/ipnet 1
pkg/rhcos 0
pkg/terraform 1
pkg/types 50My impression was that The new |
|
/approve |
Stop relying on field promotion [1], because I'm about to add InstallConfig.Platform.Name(). [1]: https://golang.org/ref/spec#Struct_types
This removes a few Cluster properties which are no longer used by the new installer. It also drops YAML loading for Cluster now that it's a one-way map from InstallConfig to Terraform JSON. I've shifted validation into its own package, since we should be validating these at the InstallConfig level. I've also shifted the Terraform variable generation out of pkg/types, because it's not about public types. I've reduced the variable generation to a single function. I've dropped the old Cluster (and subtype) specific validation. We'll want to add InstallConfig validation once we support loading it from the disk, but for now we can rely on the UserProvided validation (and this commit was already large enough ;).
a0a1dbc to
619db92
Compare
|
Rebased onto master and green. |
abhinavdahiya
left a comment
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.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
DefaultIfName was added in f245ca6 (installer: validate libvirt, 2018-06-06, coreos/tectonic-installer#3265), but the last consumer was removed by 619db92 (pkg/types/config: Break into pkg/validate and pkg/tfvars, 2018-10-03, openshift#412). We currently feed the default bridge name in via pkg/asset/installconfig/libvirt's defaultNetworkIfName. The AWS and OpenStack default variables I'm also removing here have a similar history and current default location.
Before this commit, this section would clobber any config.AWS properties which had been set up while iterating over machine pools. I'd introduced that bug in 619db92 (pkg/types/config: Break into pkg/validate and pkg/tfvars, 2018-10-03, openshift#412), before which this section ran before the machine-pool iteration, so there had been nothing to clobber.
This seems to be a relic of 619db92 (pkg/types/config: Break into pkg/validate and pkg/tfvars, 2018-10-03, openshift#412) and 5192edd (Add OpenStack OWNERS, 2018-10-06, openshift#428) coming in close proximity.
This seems to be a relic of 619db92 (pkg/types/config: Break into pkg/validate and pkg/tfvars, 2018-10-03, openshift#412) and 5192edd (Add OpenStack OWNERS, 2018-10-06, openshift#428) coming in close proximity.
Builds on #403; I'll rebase after that lands.
This removes a few
Clusterproperties which are no longer used by the new installer. It also drops YAML loading for Cluster now that it's a one-way map fromInstallConfigto Terraform JSON. I've shifted validation into its own package, since we should be validating these at theInstallConfiglevel.I've dropped the old
Cluster(and subtype) specific validation. We'll want to addInstallConfigvalidation once we support loading it from the disk, but for now we can rely on theUserProvidedvalidation (and this commit was already large enough ;).