Deprecate EtcdDiscoveryDomain field on ControllerConfig CRD - #1675
Conversation
2c63fcb to
61c11d4
Compare
|
/assign hexfusion |
|
Looks sane. Could you update the typo in the commit message/title /test e2e-vsphere |
61c11d4 to
5cc8813
Compare
LorbusChris
changed the title
|
whoopsie. fixed :) |
|
Also note that https://github.com/openshift/machine-config-operator/blob/master/pkg/operator/render.go#L98 currently isn't touched by this, so the field is still going to be populated. Maybe I should add a comment there noting that it is deprecated and can eventually be removed one day? Edit: Or would you rather have this removed from the rendered spec already? |
|
/retest |
I'm in favor of adding a comment saying it's unused for at least a release (even if we're really the one using that) - no biggie, we'll remove it afterward, what matters here is that we turned to use the infra one which is the new and correct one 👍 |
|
Will likely go in in 4.6 as it's not a bug, the valid bug will prevent this from merging /approve |
openshift-ci-robot
added
the
approved
5cc8813 to
752338a
Compare
|
Added the comment |
|
/test e2e-gcp-op |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
/lgtm
/hold until 4.6 just in case the BZ field slips for some reason
openshift-ci-robot
added
the
do-not-merge/hold
openshift-ci-robot
added
lgtm
Currently ControllerConfig.EtcdDiscoveryDomain and ControllerConfig.Infra.Status.EtcdDiscoveryDomain are redundant. This deprecates the ControllerConfig.EtcdDiscoveryDomain field and leaves ControllerConfig.Infra.Status.EtcdDiscoveryDomain as the canonical location.
752338a to
c3bf0fd
Compare
openshift-ci-robot
removed
lgtm
|
PR rebased. /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LorbusChris, runcom, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-metal-ipi |
|
@LorbusChris: 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. DetailsInstructions 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
The EtcdDiscoveryDomain field on ControllerConfig CRD was deprecated in [1] in favor of `ControllerConfig.Infra.Status.EtcdDiscoveryDomain`. However the originial PR missed two matches. This fixes it so that there is no surprise in case `ControllerConfig.EtcdDiscoveryDomain` is effectively removed. [1] openshift#1675
Deprecate EtcdDiscoveryDomain field on ControllerConfig CRD
The EtcdDiscoveryDomain field on ControllerConfig CRD was deprecated in [1] in favor of `ControllerConfig.Infra.Status.EtcdDiscoveryDomain`. However the originial PR missed two matches. This fixes it so that there is no surprise in case `ControllerConfig.EtcdDiscoveryDomain` is effectively removed. [1] openshift#1675
8 participants
Currently ControllerConfig.EtcdDiscoveryDomain and
ControllerConfig.Infra.Status.EtcdDiscoveryDomain are redundant..
- What I did
This deprecates the ControllerConfig.EtcdDiscoveryDomain field and
leaves ControllerConfig.Infra.Status.EtcdDiscoveryDomain as the
canonical location.
- How to verify it
CI
- Description for the changelog
Deprecate EtcdDiscvoeryDomain field on ControllerConfig CRD