assets/installconfig: apply defaults to install config #902

staebler · 2018-12-13T21:44:18Z

The Install Config asset will apply defaults to fields for which there are reasonable defaults. This applies to a generated Install Config asset and to an Install Config asset loaded from disk.

Note that the ClusterID asset has been removed in favor of generating a value for ClusterID when applying defaults. This is not meant to supersede any active work to remove the ClusterID from the install config. It is only meant to provide a temporary means by which a user can supply an install-config.yml without a ClusterID.

This is the minimum file needed for an AWS install-config.yml.

baseDomain: <<your base domain>>
metadata:
  name: <<your cluster name>>
platform:
  aws:
    region: <<aws region>>
pullSecret: <<your pull secret>>
sshKey: <<your ssh key>>

This is the minimum file needed for a Libvirt install-config.yml.

baseDomain: <<your base domain>>
metadata:
  name: <<your cluster name>>
platform:
  libvirt:
    URI: <<your libvirtd uri>>
pullSecret: <<your pull secret>>
sshKey: <<your ssh key>>

An invocation of openshift-install create install-config will output these minimum files (with the addition of the superfluous .metadata.creationTimestamp).

pkg/types/installconfig.go

abhinavdahiya · 2018-12-13T22:14:26Z

@staebler this is 💯 🔥
just a comment nit.

/approve

cgwalters · 2018-12-20T13:30:47Z

It looks like this should be complementary with #856 in that libvirt users should no longer need to specify the URI.

EDIT: Nope, this is a more general and better PR! However I think the improvements in 856 are still useful, but I'll rebase it after this lands.

abhinavdahiya · 2018-12-20T23:09:37Z

pkg/asset/installconfig/installconfig.go

@@ -124,6 +78,16 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
 		Data:     data,
 	}

+	// Apply the defaults *after* marshaling into yaml so that the defaults


I am a little concerned. So the asset has 2 values of the install-config. one in File and other in Config.
And in the past people have tried to use Files() on an asset and decode rather than using the type.

Also, another thing which is more of a personal preference.
getting the install-config with values defaults is a good representation of what the installer has chosen for you and what you can modify for yourself.

^^ thoughts @crawford @wking @cgwalters @staebler

Hrmmmmmmmmmmm. I think I'd prefer if the install-config contained the defaults. Can we do any tricks to comment out the defaults but still include them?

Can we do any tricks to comment out the defaults but still include them?

This sounds really complicated.

As long as the installer handles default-injection after loading an install-config, I don't really care whether we include defaults in configs we write to disk or not.

Can we change this so that the defaults are included? We can provide documentation to show which fields can be safely omitted for users who want to slim down.

Sure. I'll make that change right after I put the kids to bed.

cgwalters · 2019-01-03T16:18:55Z

@staebler are you planning to rebase this?

I think being able to reuse an install config but still fetch the latest RHCOS is absolutely essential for development.

The Install Config asset will apply defaults to fields for which there are reasonable defaults. This applies to a generated Install Config asset and to an Install Config asset loaded from disk.

staebler · 2019-01-10T21:22:11Z

/retest

The installer will include the default values that it supplies in the install-config.yaml written to disk when targeting install-config.

wking · 2019-01-12T00:35:17Z

/retest

abhinavdahiya · 2019-01-12T00:48:53Z

/lgtm

openshift-ci-robot · 2019-01-12T00:49:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,staebler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

crawford · 2019-01-12T03:24:03Z

A lot of failures due to etcd being unavailable...

/retest

wking · 2019-01-12T06:02:19Z

This conflicts with the just-landed #1052. I'll rebase on top.

/close

openshift-ci-robot · 2019-01-12T06:05:54Z

@staebler: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`6c8f95c`	link	`/test e2e-aws`
ci/prow/unit	`6c8f95c`	link	`/test unit`
ci/prow/shellcheck	`6c8f95c`	link	`/test shellcheck`
ci/prow/tf-fmt	`6c8f95c`	link	`/test tf-fmt`
ci/prow/tf-lint	`6c8f95c`	link	`/test tf-lint`
ci/prow/yaml-lint	`6c8f95c`	link	`/test yaml-lint`
ci/prow/govet	`6c8f95c`	link	`/test govet`
ci/prow/images	`6c8f95c`	link	`/test images`
ci/prow/rhel-images	`6c8f95c`	link	`/test rhel-images`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2019-01-12T06:55:10Z

Carried with #1058

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 13, 2018

openshift-ci-robot requested review from hardys and steveej December 13, 2018 21:44

abhinavdahiya reviewed Dec 13, 2018

View reviewed changes

pkg/types/installconfig.go Outdated Show resolved Hide resolved

cgwalters mentioned this pull request Dec 20, 2018

Refactor RHCOS fetch API, support fetching qemu from partial config #856

Closed

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2018

abhinavdahiya reviewed Dec 20, 2018

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2018

This was referenced Jan 4, 2019

pkg/types: Add cross-platform Networking.MachineCIDR #983

Merged

Recommend regeneration of install-config.yaml on upgrade #1016

Closed

crawford added this to the Freeze milestone Jan 10, 2019

assets/installconfig: apply defaults to install config

b72a736

The Install Config asset will apply defaults to fields for which there are reasonable defaults. This applies to a generated Install Config asset and to an Install Config asset loaded from disk.

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 10, 2019

asset/installconfig: include defaults in install-config.yaml

6c8f95c

The installer will include the default values that it supplies in the install-config.yaml written to disk when targeting install-config.

openshift-ci-robot assigned abhinavdahiya Jan 12, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2019

wking mentioned this pull request Jan 12, 2019

assets/installconfig: apply defaults to install config #1058

Merged

wking closed this Jan 12, 2019

wking mentioned this pull request Jan 30, 2019

updates to the custom install draft openshift/openshift-docs#13468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assets/installconfig: apply defaults to install config #902

assets/installconfig: apply defaults to install config #902

staebler commented Dec 13, 2018 •

edited

Loading

abhinavdahiya commented Dec 13, 2018

cgwalters commented Dec 20, 2018 •

edited

Loading

abhinavdahiya Dec 20, 2018

abhinavdahiya Dec 20, 2018 •

edited

Loading

crawford Jan 4, 2019

wking Jan 4, 2019 •

edited

Loading

crawford Jan 10, 2019

staebler Jan 11, 2019

cgwalters commented Jan 3, 2019

staebler commented Jan 10, 2019

wking commented Jan 12, 2019

abhinavdahiya commented Jan 12, 2019

openshift-ci-robot commented Jan 12, 2019

crawford commented Jan 12, 2019

wking commented Jan 12, 2019 •

edited

Loading

openshift-ci-robot commented Jan 12, 2019

wking commented Jan 12, 2019

assets/installconfig: apply defaults to install config #902

assets/installconfig: apply defaults to install config #902

Conversation

staebler commented Dec 13, 2018 • edited Loading

abhinavdahiya commented Dec 13, 2018

cgwalters commented Dec 20, 2018 • edited Loading

abhinavdahiya Dec 20, 2018

Choose a reason for hiding this comment

abhinavdahiya Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

crawford Jan 4, 2019

Choose a reason for hiding this comment

wking Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

crawford Jan 10, 2019

Choose a reason for hiding this comment

staebler Jan 11, 2019

Choose a reason for hiding this comment

cgwalters commented Jan 3, 2019

staebler commented Jan 10, 2019

wking commented Jan 12, 2019

abhinavdahiya commented Jan 12, 2019

openshift-ci-robot commented Jan 12, 2019

crawford commented Jan 12, 2019

wking commented Jan 12, 2019 • edited Loading

openshift-ci-robot commented Jan 12, 2019

wking commented Jan 12, 2019

staebler commented Dec 13, 2018 •

edited

Loading

cgwalters commented Dec 20, 2018 •

edited

Loading

abhinavdahiya Dec 20, 2018 •

edited

Loading

wking Jan 4, 2019 •

edited

Loading

wking commented Jan 12, 2019 •

edited

Loading