Bug 2037813: [baremetal] networkConfig field now accepts yaml instead of string value #5545

andfasano · 2022-01-18T14:26:43Z

The current patch allows the user to specify the content of the install-config networkConfig field directly as a yaml object. Content validation (for a generic yaml) is now carried on by the install config asset

/label platform/baremetal

openshift-ci · 2022-01-18T14:26:55Z

@andfasano: This pull request references Bugzilla bug 2037813, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

Details

In response to this:

Bug 2037813: [baremeta] networkConfig field now accepts yaml instead of string value

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.

staebler

The current patch allows the user to specify the content of the install-config networkConfig field directly as a yaml object.

Hooray for this!

pkg/asset/machines/baremetal/hosts.go

staebler · 2022-01-18T15:57:54Z

/approve

zaneb · 2022-01-18T16:47:31Z

Unfortunate that we can't allow a string as well, but it appears to me that would be banned by rule 3 of https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema

/approve

openshift-ci · 2022-01-18T16:48:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [staebler]

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

staebler · 2022-01-18T16:52:47Z

Unfortunate that we can't allow a string as well, but it appears to me that would be banned by rule 3 of https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema

/approve

You could have two separate fields--one that takes yaml and one that takes a string. The installer would fill out the one that is easier to use internally with the value from the other, if set. The installer would need to validate that if both are set that they are equivalent.

sadasu · 2022-01-18T16:54:34Z

/lgtm

Nice improvement.

andfasano · 2022-01-18T16:55:22Z

/hold

let's wait for the results of the pending jobs

andfasano · 2022-01-19T13:31:37Z

/test e2e-metal-ipi-ovn-ipv6-required
/test e2e-metal-ipi-virtualmedia

zaneb · 2022-01-19T14:12:41Z

You could have two separate fields--one that takes yaml and one that takes a string. The installer would fill out the one that is easier to use internally with the value from the other, if set. The installer would need to validate that if both are set that they are equivalent.

The main reason to support both would be so that we don't have to worry about users doing it wrong, so I think having two fields is just giving people twice as many ways to mess up :)

andfasano · 2022-01-19T18:06:53Z

/hold cancel
metal-ipi jobs went fine

Agree also on just keeping one field for the sake of simplicity

openshift-bot · 2022-01-19T19:28:30Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-19T20:07:34Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-19T22:56:31Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-19T23:08:54Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-19T23:56:29Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T02:20:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T05:09:49Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T07:27:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T07:39:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T08:51:53Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T10:03:50Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T11:03:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-20T11:15:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

andfasano · 2022-01-20T12:26:04Z

There was a problem with the controller-gen, since interface{} type wasn't allowed, so I had to switch type.

pkg/asset/machines/baremetal/hosts.go

andfasano · 2022-01-20T14:07:26Z

/test verify-vendor

staebler · 2022-01-20T15:09:56Z

pkg/types/baremetal/platform.go

I guess the plus side of this is that you could accept a string here as well. I don't know if we want to, but you could.

pkg/asset/machines/baremetal/hosts_test.go

pkg/types/baremetal/validation/platform.go

The current patch allows the user to specify the content of the install-config networkConfig field directly as a yaml object. Content validation (for a generic yaml) is now carried on by the install config asset

sadasu · 2022-01-21T13:57:14Z

/lgtm

andfasano · 2022-01-21T14:02:06Z

/hold
Checking ipv6 job failure

andfasano · 2022-01-21T14:19:01Z

/hold cancel

/test e2e-metal-ipi-ovn-ipv6-required

openshift-bot · 2022-01-21T15:28:34Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-01-21T17:16:37Z

@andfasano: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6	`8b3d14d`	link	false	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/okd-e2e-aws-upgrade	`8b3d14d`	link	false	`/test okd-e2e-aws-upgrade`
ci/prow/e2e-metal-ipi-ovn-dualstack	`8b3d14d`	link	false	`/test e2e-metal-ipi-ovn-dualstack`
ci/prow/e2e-azure-upi	`8b3d14d`	link	false	`/test e2e-azure-upi`
ci/prow/e2e-aws-workers-rhel8	`8b3d14d`	link	false	`/test e2e-aws-workers-rhel8`
ci/prow/e2e-metal-ipi-virtualmedia	`8b3d14d`	link	false	`/test e2e-metal-ipi-virtualmedia`
ci/prow/e2e-aws-single-node	`8b3d14d`	link	false	`/test e2e-aws-single-node`
ci/prow/okd-e2e-aws	`8b3d14d`	link	false	`/test okd-e2e-aws`
ci/prow/e2e-aws-workers-rhel7	`8b3d14d`	link	false	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-crc	`8b3d14d`	link	false	`/test e2e-crc`
ci/prow/e2e-ibmcloud	`8b3d14d`	link	false	`/test e2e-ibmcloud`

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci · 2022-01-21T17:17:32Z

@andfasano: All pull requests linked via external trackers have merged:

openshift/installer#5545

Bugzilla bug 2037813 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2037813: [baremetal] networkConfig field now accepts yaml instead of string value

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.

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 18, 2022

andfasano changed the title ~~Bug 2037813: [baremeta] networkConfig field now accepts yaml instead of string value~~ Bug 2037813: [baremetal] networkConfig field now accepts yaml instead of string value Jan 18, 2022

openshift-ci bot added the platform/baremetal IPI bare metal hosts platform label Jan 18, 2022

openshift-ci bot requested review from hardys and rna-afk January 18, 2022 14:33

staebler reviewed Jan 18, 2022

View reviewed changes

pkg/asset/machines/baremetal/hosts.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2022

andfasano force-pushed the nmstate-pure-yaml branch from bde6afd to a0a3c22 Compare January 18, 2022 16:16

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2022

openshift-ci bot assigned sadasu Jan 18, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2022

andfasano force-pushed the nmstate-pure-yaml branch from a0a3c22 to e23e547 Compare January 20, 2022 12:22

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

ardaguclu reviewed Jan 20, 2022

View reviewed changes

pkg/asset/machines/baremetal/hosts.go Outdated Show resolved Hide resolved

andfasano force-pushed the nmstate-pure-yaml branch from e23e547 to cbd51fd Compare January 20, 2022 14:40

staebler reviewed Jan 20, 2022

View reviewed changes

pkg/asset/machines/baremetal/hosts_test.go Outdated Show resolved Hide resolved

pkg/types/baremetal/validation/platform.go Outdated Show resolved Hide resolved

baremetal: networkConfig field now accepts yaml instead of string value

8b3d14d

The current patch allows the user to specify the content of the install-config networkConfig field directly as a yaml object. Content validation (for a generic yaml) is now carried on by the install config asset

andfasano force-pushed the nmstate-pure-yaml branch from cbd51fd to 8b3d14d Compare January 20, 2022 18:19

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2022

openshift-merge-robot merged commit 969378d into openshift:master Jan 21, 2022

Bug 2037813: [baremetal] networkConfig field now accepts yaml instead of string value #5545

Bug 2037813: [baremetal] networkConfig field now accepts yaml instead of string value #5545

Uh oh!

Conversation

andfasano commented Jan 18, 2022

Uh oh!

openshift-ci bot commented Jan 18, 2022

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

staebler commented Jan 18, 2022

Uh oh!

zaneb commented Jan 18, 2022

Uh oh!

openshift-ci bot commented Jan 18, 2022

Uh oh!

staebler commented Jan 18, 2022

Uh oh!

sadasu commented Jan 18, 2022

Uh oh!

andfasano commented Jan 18, 2022

Uh oh!

andfasano commented Jan 19, 2022

Uh oh!

zaneb commented Jan 19, 2022

Uh oh!

andfasano commented Jan 19, 2022

Uh oh!

openshift-bot commented Jan 19, 2022

Uh oh!

openshift-bot commented Jan 19, 2022

Uh oh!

openshift-bot commented Jan 19, 2022

Uh oh!

openshift-bot commented Jan 19, 2022

Uh oh!

openshift-bot commented Jan 19, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

openshift-bot commented Jan 20, 2022

Uh oh!

andfasano commented Jan 20, 2022

Uh oh!

Uh oh!

andfasano commented Jan 20, 2022

Uh oh!

staebler Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sadasu commented Jan 21, 2022

Uh oh!

andfasano commented Jan 21, 2022

Uh oh!

andfasano commented Jan 21, 2022

Uh oh!

openshift-bot commented Jan 21, 2022

Uh oh!

openshift-ci bot commented Jan 21, 2022

Uh oh!

openshift-ci bot commented Jan 21, 2022

Uh oh!

Reviewers

Assignees