pkg/operator: Remove infra platform string constants #814

sgreene570 · 2019-06-04T20:47:11Z

- What I did

The openshift v1 infrastructure API types have been updated (https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go). As is, render.go does not take into account bare metal or GCP cluster type strings. The currently used struct fields are also marked as deprecated in the API source code.

- How to verify it

Should not be semantically different.

- Description for the changelog

Update platform type strings in controller config spec

sgreene570 · 2019-06-04T21:46:18Z

/retest

runcom · 2019-06-04T21:55:10Z

pkg/operator/render.go

-	case configv1.VSpherePlatformType:
-		platform = "vsphere"
-	}
+	platform := strings.ToLower(string(infra.Status.PlatformStatus.Type))


the only semantic difference is that we don't fallback to a none platform right? cc @abhinavdahiya for confirmation

actually looks like the none platform is defined? @runcom not sure if that's used by installer though.
https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go

oh great, nevermind this comment then

question: is there any way the platform won't be set to anything (not None, but empty)? maybe we should check to make sure it has a value?

runcom · 2019-06-04T21:55:31Z

Looks great as is, just a friendly ping to installer 👍

pkg/operator/render.go

runcom · 2019-06-04T22:15:05Z

gosh, fat fingers (wanted to comment)

/approve
/lgtm

openshift-bot · 2019-06-04T22:34:19Z

/retest

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

openshift-bot · 2019-06-04T23:00:25Z

/retest

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

openshift-bot · 2019-06-05T09:14:29Z

/retest

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

runcom · 2019-06-05T10:03:44Z

uhm, something isn't properly working here

/hold

kikisdeliveryservice

Is the intention of this PR to add support for additional platforms? For example, the previous state put baremetal, GCP, libvirt to None as opposed to passing them as actual platform values. I'm pretty sure this is why the switch statement was originally used and that we should make sure we actually want to expand the potential platform values that we are passing before we merge this.

See: https://github.com/openshift/api/blob/43c075c38fd931d1c85b41039e79caac63fc1925/config/v1/types_infrastructure.go#L70

@runcom

runcom · 2019-06-05T16:49:49Z

Is the intention of this PR to add support for additional platforms? For example, the previous state put baremetal, GCP, libvirt to None as opposed to passing them as actual platform values. I'm pretty sure this is why the switch statement was originally used and that we should make sure we actually want to expand the potential platform values that we are passing before we merge this.

See: https://github.com/openshift/api/blob/43c075c38fd931d1c85b41039e79caac63fc1925/config/v1/types_infrastructure.go#L70

@runcom

4.2 supports that set of platforms so we should be good landing this to master (cross checked with Ian as well)

kikisdeliveryservice · 2019-06-05T16:51:16Z

4.2 supports that set of platforms so we should be good landing this to master (cross checked with Ian as well)

is there any issue merging here before they are fully supported?

runcom · 2019-06-05T16:52:01Z

s there any issue merging here before they are fully supported?

nope afaict, the installer sets the platform and if someone is using master is not something released so free to break anytime w/o support.

kikisdeliveryservice · 2019-06-05T16:54:14Z

nope afaict, the installer sets the platform and if someone is using master is not something released so free to break anytime w/o support.

Fair enough! 😄

Just 1 comment from me above, as I'd like a check that platform has a value otherwise set to None: #814 (comment)

runcom · 2019-06-05T17:08:02Z

/lgtm cancel

keep holding this PR because the PlatformStatus is still empty because we need openshift/installer#1725 (comment). We can go ahead with this PR once the installer PR lands

sgreene570 · 2019-06-05T17:21:28Z

Added empty string check so this will be ready once the installer PR is merged.

pkg/operator/render.go

sgreene570 · 2019-06-14T22:52:01Z

Added a unit test for the function I modified 😃

runcom · 2019-07-03T08:34:34Z

/retest

runcom · 2019-07-03T08:35:41Z

/approve
/hold cancel

pkg/operator/render.go

runcom · 2019-07-03T10:34:31Z

/retest

pkg/operator/render.go

kikisdeliveryservice · 2019-07-03T16:59:37Z

verify isn't allowing the new check bc the type is deprecated here: https://github.com/openshift/api/blob/49f610c1a3a8461a9d0623ad35fa08d943b12c68/config/v1/types_infrastructure.go#L42

runcom · 2019-07-03T17:22:08Z

Let's just add a //nolint:linter_name on top of it to silence the l'Inter - standard practice

kikisdeliveryservice · 2019-07-03T17:29:00Z

Let's just add a //nolint:linter_name on top of it to silence the l'Inter - standard practice

nice!! i thought that was the way but wasn't sure. just saw similar here: https://github.com/openshift/machine-config-operator/pull/918/files#diff-fa45321336db7ad1cedc28bf643a4f97

openshift-ci-robot · 2019-07-03T20:15:38Z

@sgreene570: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-etcd-quorum-loss	41b59b56cd6b786f7ac52a7e8f2145cb6d07ea06	link	`/test e2e-etcd-quorum-loss`

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.

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.

kikisdeliveryservice · 2019-07-03T20:16:59Z

/skip

kikisdeliveryservice · 2019-07-03T20:18:05Z

i think we're just probably waiting for that e2e test to stop flaking

sgreene570 · 2019-07-09T17:50:02Z

/retest

runcom · 2019-07-10T10:38:24Z

/lgtm

openshift-ci-robot · 2019-07-10T10:38:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: runcom, sgreene570

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

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

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 4, 2019

openshift-ci-robot requested review from LorbusChris and cgwalters June 4, 2019 20:47

runcom reviewed Jun 4, 2019

View reviewed changes

abhinavdahiya reviewed Jun 4, 2019

View reviewed changes

pkg/operator/render.go Outdated Show resolved Hide resolved

sgreene570 force-pushed the platform-info-api-fix branch from 849ae0a to a4bada1 Compare June 4, 2019 22:10

runcom closed this Jun 4, 2019

runcom reopened this Jun 4, 2019

openshift-ci-robot assigned runcom Jun 4, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 4, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2019

kikisdeliveryservice reviewed Jun 5, 2019

View reviewed changes

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 5, 2019

sgreene570 force-pushed the platform-info-api-fix branch from a4bada1 to 41b59b5 Compare June 5, 2019 17:17

kikisdeliveryservice reviewed Jun 5, 2019

View reviewed changes

pkg/operator/render.go Outdated Show resolved Hide resolved

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

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

sgreene570 force-pushed the platform-info-api-fix branch from 41dfa81 to 7d3a890 Compare July 2, 2019 22:03

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

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 3, 2019

runcom reviewed Jul 3, 2019

View reviewed changes

pkg/operator/render.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Jul 3, 2019

View reviewed changes

pkg/operator/render.go Outdated Show resolved Hide resolved

sgreene570 force-pushed the platform-info-api-fix branch from 7d3a890 to 155f738 Compare July 3, 2019 16:31

runcom assigned kikisdeliveryservice and unassigned runcom Jul 3, 2019

sgreene570 added 2 commits July 3, 2019 10:42

pkg/operator: Remove infra platform string constants

cb3d06a

pkg/operator: add render.go unit tests

e0b06c4

sgreene570 force-pushed the platform-info-api-fix branch from 155f738 to e0b06c4 Compare July 3, 2019 17:42

openshift-ci-robot assigned runcom Jul 10, 2019

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

openshift-merge-robot merged commit 53d9f47 into openshift:master Jul 10, 2019

pkg/operator: Remove infra platform string constants #814

pkg/operator: Remove infra platform string constants #814

Uh oh!

Conversation

sgreene570 commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgreene570 commented Jun 4, 2019

Uh oh!

runcom Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

sgreene570 Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

runcom Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

kikisdeliveryservice Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

runcom commented Jun 4, 2019

Uh oh!

Uh oh!

runcom commented Jun 4, 2019

Uh oh!

openshift-bot commented Jun 4, 2019

Uh oh!

openshift-bot commented Jun 4, 2019

Uh oh!

openshift-bot commented Jun 5, 2019

Uh oh!

runcom commented Jun 5, 2019

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

runcom commented Jun 5, 2019

Uh oh!

kikisdeliveryservice commented Jun 5, 2019

Uh oh!

runcom commented Jun 5, 2019

Uh oh!

kikisdeliveryservice commented Jun 5, 2019

Uh oh!

runcom commented Jun 5, 2019

Uh oh!

sgreene570 commented Jun 5, 2019

Uh oh!

Uh oh!

sgreene570 commented Jun 14, 2019

Uh oh!

runcom commented Jul 3, 2019

Uh oh!

runcom commented Jul 3, 2019

Uh oh!

Uh oh!

runcom commented Jul 3, 2019

Uh oh!

Uh oh!

kikisdeliveryservice commented Jul 3, 2019

Uh oh!

runcom commented Jul 3, 2019

Uh oh!

kikisdeliveryservice commented Jul 3, 2019

Uh oh!

openshift-ci-robot commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikisdeliveryservice commented Jul 3, 2019

Uh oh!

kikisdeliveryservice commented Jul 3, 2019

Uh oh!

sgreene570 commented Jul 9, 2019

Uh oh!

runcom commented Jul 10, 2019

Uh oh!

openshift-ci-robot commented Jul 10, 2019

Uh oh!

Reviewers

sgreene570 commented Jun 4, 2019 •

edited

Loading

openshift-ci-robot commented Jul 3, 2019 •

edited

Loading