Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1990012: Update controller config openapi schema #2702

Merged

Conversation

JoelSpeed
Copy link
Contributor

- What I did

I used controller-gen to regenerate the openAPIV3Schema section of this CRD.
This means that it is now up to date and includes all of the latest fields from the objects that are embedded within it.

- How to verify it
Find a field such as .spec.infra.status.platformStatus.azure.cloudName which is not currently present in the CRD schema. Observe how this exists in the infrastructure resource, but does not exist in the embedded infra in the controllerconfig in clusters today. Try to add this field into the controllerconfig, it will be silently dropped by the API server.
Upgrade to this patch, observe how the field now exists and is copied from the main infrastructure resource.

- Description for the changelog

Update OpenAPI schema for controllerconfig CRD

@JoelSpeed JoelSpeed changed the title Update controller config openapi schema Bug 1990012: Update controller config openapi schema Aug 4, 2021
@openshift-ci openshift-ci bot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Aug 4, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 4, 2021

@JoelSpeed: This pull request references Bugzilla bug 1990012, 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.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rioliu@redhat.com), skipping review request.

In response to this:

Bug 1990012: Update controller config openapi schema

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 openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 4, 2021
@JoelSpeed JoelSpeed force-pushed the update-controller-config-crd branch from e63680e to 0e99632 Compare August 4, 2021 15:12
@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Aug 4, 2021

Sinny has been looking at moving our API to openshift/api, so looping her in to take a look.

Could you also provide some steps you used to generate this? I tried some time ago to use the generator and wasn't fully successful, and I think recently Sinny also had to do some manual steps to update this. Might be good to have it documented in our hacking doc for future reference.

@JoelSpeed
Copy link
Contributor Author

Sure, we have a script that runs this for us in MAO, so I originally started by copying that and trying to make it work for your project as I noticed you don't have a script for doing it.

The problem I reached was that your machineconfig has a raw extension, currently you have a schema defined within this even though it's not needed, so there's a large diff on the CR, everything else is pretty much ok and as normal

You can see the progress I made here JoelSpeed@8769284

Theres a few changes I had to make in pkg/apis that you may want to take note of, in particular the groupName comment is required to get controller-gen to do anything at all :)

@patrickdillon
Copy link
Contributor

/test e2e-azure

@lobziik built a release image built with this patch. It resolves issues for joining worker nodes in Azure Stack UPI.

@kikisdeliveryservice
Copy link
Contributor

/test e2e-agnostic-upgrade

@JoelSpeed
Copy link
Contributor Author

/test e2e-agnostic-upgrade

Is there anything more we need to do for this?

@JoelSpeed
Copy link
Contributor Author

/test e2e-agnostic-upgrade

Copy link
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.

Upgrade test failures seem unrelated to this PR. Generally lgtm, will give others a chance to take a last look if any concerns.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2021
@sinnykumari
Copy link
Contributor

Thanks Joel for catching this issue and opening up PR!
As Jerry said, so far we have been updating our CRDs manually and we missed these changes as it wasn't directly MCO needed so far.

You can see the progress I made here JoelSpeed@8769284

Theres a few changes I had to make in pkg/apis that you may want to take note of, in particular the groupName comment is required to get controller-gen to do anything at all :)

+1, hopefully we will not have these issues once we finish our work to move MCO API to openshift/api

/lgtm

@sinnykumari
Copy link
Contributor

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 12, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, sinnykumari, yuqi-zhang

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 [sinnykumari,yuqi-zhang]

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
Contributor

/retest-required

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

8 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

16 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 15, 2021

@JoelSpeed: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-single-node 0e99632 link /test e2e-aws-single-node
ci/prow/e2e-aws-techpreview-featuregate 0e99632 link /test e2e-aws-techpreview-featuregate
ci/prow/e2e-aws-workers-rhel7 0e99632 link /test e2e-aws-workers-rhel7
ci/prow/e2e-aws-disruptive 0e99632 link /test e2e-aws-disruptive
ci/prow/e2e-aws-upgrade-single-node 0e99632 link /test e2e-aws-upgrade-single-node

Full PR test history. Your PR dashboard.

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
Contributor

/retest-required

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

6 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-ci openshift-ci bot merged commit 9f7f2f2 into openshift:master Aug 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 15, 2021

@JoelSpeed: All pull requests linked via external trackers have merged:

Bugzilla bug 1990012 has been moved to the MODIFIED state.

In response to this:

Bug 1990012: Update controller config openapi schema

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants