Skip to content

Deprecate Platform field on ControllerConfig CRD#1624

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
LorbusChris:platformstatus
Jun 23, 2020
Merged

Deprecate Platform field on ControllerConfig CRD#1624
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
LorbusChris:platformstatus

Conversation

@LorbusChris
Copy link
Copy Markdown
Contributor

@LorbusChris LorbusChris commented Apr 6, 2020 •
edited
Loading

Currently ControllerConfig.Platform and
ControllerConfig.Infra.Status.PlatformStatus.Type contain redundant
information. However, the former is encoded as untyped string and the
latter as configv1.PlatformType.

This deprecates the ControllerConfig.Platform field and leaves
ControllerConfig.Infra.Status.PlatformStatus.Type as the
canonical platform identifier, making all internals consume that field
instead.

@LorbusChris LorbusChris requested review from runcom and yuqi-zhang April 6, 2020 19:05
@LorbusChris LorbusChris force-pushed the platformstatus branch 2 times, most recently from 2d825c7 to 294a9fe Compare April 6, 2020 19:31
LorbusChris
LorbusChris commented Apr 6, 2020
View reviewed changes
Comment thread pkg/controller/template/render.go Outdated
yuqi-zhang
yuqi-zhang reviewed Apr 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally in favour of removing deprecated fields, and this looks sane at a first pass. Will let others take a look and do a more through review.

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Apr 6, 2020

I've rephrased this a little. Deprecated probably wasn't quite true as this is still used right now.
The info however is redundant, and it's not typed as-is (the Infra.Status.PlatformStatus.Type field is typed)

As this is an internal API the change shouldn't cause any problems.

Btw, there is another redundancy: ControllerConfig.EtcdDiscoveryDomain and ControllerConfig.Infra.Status.EtcdDiscoveryDomain. I feel like this should also be deduped and canonicalized in Infra.Status

@LorbusChris LorbusChris force-pushed the platformstatus branch 2 times, most recently from eb07634 to 180090c Compare April 7, 2020 10:49
@LorbusChris LorbusChris changed the title Remove Platform field from ControllerConfig CRD Dedupe ControllerConfig CRD fields and clean up import names Apr 7, 2020
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Apr 7, 2020

/retest

@LorbusChris LorbusChris force-pushed the platformstatus branch 2 times, most recently from 7267a7c to f441d05 Compare April 8, 2020 23:43
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Apr 9, 2020

/test images

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Apr 9, 2020

/retest

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Apr 9, 2020

/cc @runcom
Updated per your suggestions, ptal when you find the time!

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2020
@LorbusChris LorbusChris changed the title Dedupe ControllerConfig CRD fields and clean up import names Deprecate Platform field on ControllerConfig CRD Apr 23, 2020
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Apr 23, 2020

depends on #1658
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2020
@LorbusChris LorbusChris force-pushed the platformstatus branch 2 times, most recently from d18e8d1 to 1809de7 Compare April 27, 2020 15:23
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 4, 2020

PR rebased.

/hold cancel
/cc @yuqi-zhang @runcom

runcom
runcom reviewed Jun 4, 2020
View reviewed changes
Comment thread lib/resourcemerge/machineconfig.go Outdated
@runcom
Copy link
Copy Markdown
Member

runcom commented Jun 4, 2020

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2020
runcom
runcom reviewed Jun 4, 2020
View reviewed changes
Comment thread lib/resourcemerge/machineconfig.go Outdated
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 5, 2020

/retest

3 similar comments
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 5, 2020

/retest

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 5, 2020

/retest

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 9, 2020

/retest

@runcom
Copy link
Copy Markdown
Member

runcom commented Jun 10, 2020

needs a rebase 👼

@LorbusChris
Deprecate Platform field on ControllerConfig CRD
215162f 
Currently ControllerConfig.Platform and
ControllerConfig.Infra.Status.PlatformStatus.Type contain redundant
information. However, the former is encoded as untyped string and the
latter as configv1.PlatformType.

This deprecates the ControllerConfig.Platform field and leaves
ControllerConfig.Infra.Status.PlatformStatus.Type as the
canonical platform identifier, making all internals consume that field
instead.
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 10, 2020

rebased :)

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 10, 2020

/retest

1 similar comment
@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 18, 2020

/retest

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 19, 2020

/test e2e-aws

@LorbusChris
Copy link
Copy Markdown
Contributor Author

LorbusChris commented Jun 22, 2020

/retest

@runcom
Copy link
Copy Markdown
Member

runcom commented Jun 23, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Jun 23, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jun 23, 2020

/retest

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

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

kikisdeliveryservice commented Jun 23, 2020

/skip

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Jun 23, 2020 •
edited
Loading

@LorbusChris: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovn-step-registry 215162f link /test e2e-ovn-step-registry

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.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jun 23, 2020

/retest

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

1 similar comment
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jun 23, 2020

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 2413d41 into openshift:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@runcom runcom runcom left review comments

@mtrmac mtrmac Awaiting requested review from mtrmac

@umohnani8 umohnani8 Awaiting requested review from umohnani8

@yuqi-zhang yuqi-zhang Awaiting requested review from yuqi-zhang

Assignees

@runcom runcom

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@LorbusChris @runcom @openshift-ci-robot @openshift-bot @kikisdeliveryservice @yuqi-zhang @openshift-merge-robot