[wip] pkg/controller: Remove hard coded platform consts in controller template #853
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
Is this the right time to change our Platform type field to natively use configv1.PlatformType, instead of converting to string? |
Maybe it is. Would switching to the API type be much of a concern for upgrades? (My guess is no, but I'm not super sure 😂) |
|
neither am I 😄 /cc @runcom |
| platformLibvirt = string(configv1.LibvirtPlatformType) | ||
| platformNone = string(configv1.NonePlatformType) | ||
| platformVSphere = string(configv1.VSpherePlatformType) | ||
| platformBareMetal = string(configv1.BareMetalPlatformType) |
There was a problem hiding this comment.
we can get rid of these assignements and just use string(...) in the switch cases below right?
There was a problem hiding this comment.
sure, just did this for readability because the vars get used a few times
I'd wait for the dependent installer PR to land first and then we can definitely think about moving the ControllerConfig to use the right values |
openshift-ci-robot
added
the
needs-rebase
sgreene570
force-pushed
the
render-platform-vars-refactor
branch
from
03b4c78
to
c981d12
Compare
openshift-ci-robot
removed
the
needs-rebase
|
/retest |
sgreene570
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
sgreene570
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
Just realized this PR uses capitalized constants (from type defs in API) while the rest of MCO repo uses all lowercase constants. While switching to natively use configv1.PlatformType may cause some issues, it would certainly remove any capitalization / formatting questions. |
sgreene570
force-pushed
the
render-platform-vars-refactor
branch
3 times, most recently
from
5b75a61
to
f9acb4c
Compare
sgreene570
force-pushed
the
render-platform-vars-refactor
branch
from
f9acb4c
to
2e149fe
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sgreene570 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sgreene570
force-pushed
the
render-platform-vars-refactor
branch
3 times, most recently
from
41e9b9e
to
236c814
Compare
|
/retest |
1 similar comment
|
/retest |
openshift-ci-robot
added
the
needs-rebase
sgreene570
force-pushed
the
render-platform-vars-refactor
branch
from
236c814
to
fa0668e
Compare
openshift-ci-robot
removed
the
needs-rebase
|
/retest |
1 similar comment
|
/retest |
sgreene570
force-pushed
the
render-platform-vars-refactor
branch
from
fa0668e
to
0aa7ee3
Compare
|
@sgreene570: PR needs rebase. 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
added
the
needs-rebase
|
@sgreene570: The following tests failed, say
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. |
kikisdeliveryservice
added
the
lifecycle/stale
|
In an effort to clean up the MCO repo, closing old open PRs with no recent activity. Feel free to reopen. |
- What I did
Remove hard coded platform descriptors from
pkg/contrller/template/render.go.This PR depends on MCO #814 (which depends on openshift/installer#1725).
This may break upgrade testing. TBD.
- How to verify it
CI / run unit tests locally.
ex.
go test -v github.com/openshift/machine-config-operator/pkg/controller/template- Description for the changelog
Remove hard coded platform strings from controller template in pkg/controller