Bug 1843327: [vSphere] Improve provider spec validation #605

alexander-demicev · 2020-06-01T13:18:25Z

Return invalid machine configuration error on missing VM template and missing user data secret

elmiko

/lgtm

i just had a couple minor questions

elmiko · 2020-06-01T13:35:21Z

pkg/controller/vsphere/reconciler.go

@@ -410,7 +410,9 @@ func clone(s *machineScope) (string, error) {

 	vmTemplate, err := s.GetSession().FindVM(*s, "", s.providerSpec.Template)
 	if err != nil {
-		return "", err
+		notFoundMsg := "template not found, specify valid value"


given that notFoundMsg doesn't change, i wonder if it's better to make it a const?

Suggested change

notFoundMsg := "template not found, specify valid value"

const notFoundMsg = "template not found, specify valid value"

elmiko · 2020-06-01T13:36:51Z

pkg/controller/vsphere/reconciler.go

@@ -589,7 +591,10 @@ func getNetworkDevices(s *machineScope, devices object.VirtualDeviceList) ([]typ

 		ref, err := s.GetSession().Finder.Network(s.Context, netSpec.NetworkName)
 		if err != nil {
-			return nil, fmt.Errorf("unable to find network %q: %w", netSpec.NetworkName, err)
+			multipleFoundMsg := "multiple networks found, specify one in config"
+			notFoundMsg := "network not found, specify valid value"


similar question here for multipleFoundMsg and notFoundMsg

JoelSpeed

LGTM once Mike's suggestions are fixed up

alexander-demicev · 2020-06-17T09:00:15Z

@JoelSpeed @elmiko all fixed

/retest

enxebre · 2020-06-17T10:37:50Z

pkg/controller/vsphere/reconciler.go

 		notFoundMsg := "folder not found, specify valid value"
 		defaultError := fmt.Errorf("unable to get folder for %q: %w", folderPath, err)
 		return "", handleVSphereError(multipleFoundMsg, notFoundMsg, defaultError, err)
 	}

 	datastore, err := s.GetSession().Finder.DatastoreOrDefault(s, datastorePath)
 	if err != nil {
-		multipleFoundMsg := "multiple datastores found, specify one in config"
+		const multipleFoundMsg = "multiple datastores found, specify one in config"


why is this using const and := unconsistently??
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR469
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR470
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR594
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR595

alexander-demicev · 2020-06-30T13:43:31Z

/retest

enxebre · 2020-07-01T08:16:49Z

pkg/controller/vsphere/machine_scope.go

@@ -133,7 +134,7 @@ func (s *machineScope) GetSession() *session.Session {
 // provider spec, if one is set.
 func (s *machineScope) GetUserData() ([]byte, error) {
 	if s.providerSpec == nil || s.providerSpec.UserDataSecret == nil {
-		return nil, nil
+		return nil, machinecontroller.InvalidMachineConfiguration("user data secret is missing in provider spec")


Although I think this is fine and it makes sense as we don't currently support any model where this could be empty fwiw I don't think we are doing the same in other providers.
In the near future with a model where MCO generate secrets for machines on demand this might need to change.
openshift/enhancements#368
openshift/enhancements#201

enxebre · 2020-07-01T08:17:28Z

"Improve" seems too wide, Can we include "by returning invalid machine configuration error on missing VM template and missing user data secret" as in the desc?
/approve

enxebre · 2020-07-01T08:17:34Z

/retest

openshift-ci-robot · 2020-07-01T08:17:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

alexander-demicev · 2020-07-01T09:07:37Z

@enxebre done

openshift-ci-robot · 2020-07-01T09:13:09Z

@alexander-demichev: This pull request references Bugzilla bug 1843327, 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.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1843327: [vSphere] Improve provider spec validation

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.

alexander-demicev · 2020-07-01T13:01:44Z

/retest

… user data secret

elmiko

/lgtm

enxebre · 2020-07-02T09:08:43Z

/retest

JoelSpeed

/lgtm

openshift-bot · 2020-07-03T00:41:07Z

/retest

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

openshift-bot · 2020-07-03T00:53:55Z

/retest

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

openshift-bot · 2020-07-03T01:32:55Z

/retest

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

openshift-bot · 2020-07-03T02:24:49Z

/retest

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

openshift-bot · 2020-07-03T03:03:53Z

/retest

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

openshift-bot · 2020-07-03T04:21:51Z

/retest

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

openshift-bot · 2020-07-03T04:34:52Z

/retest

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

openshift-bot · 2020-07-03T05:26:50Z

/retest

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

openshift-bot · 2020-07-03T06:57:48Z

/retest

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

openshift-bot · 2020-07-03T07:10:49Z

/retest

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

openshift-bot · 2020-07-03T08:02:50Z

/retest

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

openshift-bot · 2020-07-03T08:15:50Z

/retest

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

openshift-bot · 2020-07-03T09:20:49Z

/retest

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

openshift-bot · 2020-07-03T10:38:48Z

/retest

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

openshift-bot · 2020-07-03T11:43:50Z

/retest

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

openshift-bot · 2020-07-03T14:45:45Z

/retest

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

openshift-bot · 2020-07-03T17:21:44Z

/retest

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

openshift-bot · 2020-07-03T19:05:46Z

/retest

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

openshift-bot · 2020-07-03T19:44:45Z

/retest

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

openshift-bot · 2020-07-03T20:23:46Z

/retest

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

openshift-bot · 2020-07-03T20:49:47Z

/retest

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

openshift-bot · 2020-07-03T22:07:48Z

/retest

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

openshift-bot · 2020-07-03T23:12:52Z

/retest

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

openshift-bot · 2020-07-04T02:01:47Z

/retest

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

openshift-bot · 2020-07-04T02:14:48Z

/retest

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

openshift-bot · 2020-07-04T04:37:46Z

/retest

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

openshift-ci-robot · 2020-07-04T05:34:44Z

@alexander-demichev: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`d1de7dd`	link	`/test e2e-aws-scaleup-rhel7`

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.

openshift-bot · 2020-07-04T05:42:44Z

/retest

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

openshift-ci-robot · 2020-07-04T06:07:45Z

@alexander-demichev: All pull requests linked via external trackers have merged: openshift/machine-api-operator#605. Bugzilla bug 1843327 has been moved to the MODIFIED state.

In response to this:

Bug 1843327: [vSphere] Improve provider spec validation

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-robot requested review from elmiko and enxebre June 1, 2020 13:18

elmiko approved these changes Jun 1, 2020

View reviewed changes

openshift-ci-robot assigned elmiko Jun 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2020

JoelSpeed reviewed Jun 2, 2020

View reviewed changes

alexander-demicev force-pushed the providerspec branch from 16e6ee8 to a865ef3 Compare June 17, 2020 08:59

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020

enxebre reviewed Jun 17, 2020

View reviewed changes

alexander-demicev force-pushed the providerspec branch from a865ef3 to 8e027a1 Compare June 30, 2020 13:24

enxebre reviewed Jul 1, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2020

alexander-demicev changed the title ~~[vSphere] Improve provider spec validation~~ Bug 1843327: [vSphere] Improve provider spec validation Jul 1, 2020

openshift-ci-robot 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 Jul 1, 2020

alexander-demicev force-pushed the providerspec branch from 8e027a1 to 43e60b6 Compare July 1, 2020 13:28

Return invalid machine configuration error on missing VM template and…

d1de7dd

… user data secret

alexander-demicev force-pushed the providerspec branch from 43e60b6 to d1de7dd Compare July 1, 2020 14:09

elmiko approved these changes Jul 1, 2020

View reviewed changes

JoelSpeed reviewed Jul 2, 2020

View reviewed changes

openshift-ci-robot assigned JoelSpeed Jul 2, 2020

openshift-merge-robot merged commit 81dfdad into openshift:master Jul 4, 2020

	notFoundMsg := "template not found, specify valid value"
	const notFoundMsg = "template not found, specify valid value"

Bug 1843327: [vSphere] Improve provider spec validation #605

Bug 1843327: [vSphere] Improve provider spec validation #605

Conversation

alexander-demicev commented Jun 1, 2020 • edited Loading

elmiko left a comment

Choose a reason for hiding this comment

elmiko Jun 1, 2020

Choose a reason for hiding this comment

elmiko Jun 1, 2020

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

alexander-demicev commented Jun 17, 2020

enxebre Jun 17, 2020

Choose a reason for hiding this comment

alexander-demicev commented Jun 30, 2020

enxebre Jul 1, 2020

Choose a reason for hiding this comment

enxebre commented Jul 1, 2020

enxebre commented Jul 1, 2020

openshift-ci-robot commented Jul 1, 2020

alexander-demicev commented Jul 1, 2020

openshift-ci-robot commented Jul 1, 2020

alexander-demicev commented Jul 1, 2020

elmiko left a comment

Choose a reason for hiding this comment

enxebre commented Jul 2, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-ci-robot commented Jul 4, 2020 • edited Loading

openshift-bot commented Jul 4, 2020

openshift-ci-robot commented Jul 4, 2020

alexander-demicev commented Jun 1, 2020 •

edited

Loading

openshift-ci-robot commented Jul 4, 2020 •

edited

Loading