Skip to content

Conversation

@joelddiaz
Copy link
Contributor

Move that logic into the codec creation util.

Move that logic into the codec creation util.
@joelddiaz joelddiaz requested a review from dgoodwin July 1, 2020 20:28
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelddiaz

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2020
@staebler
Copy link
Contributor

staebler commented Jul 1, 2020

What is the reasoning for this? Aren't the provider types part of the API? For example, isn't the AWSProviderStatus part of the cloudcredential.openshift.io/v1 API? Why wouldn't a user of the API expect the provider types to be added to the scheme when the API is added to the scheme?

@joelddiaz
Copy link
Contributor Author

What is the reasoning for this? Aren't the provider types part of the API? For example, isn't the AWSProviderStatus part of the cloudcredential.openshift.io/v1 API? Why wouldn't a user of the API expect the provider types to be added to the scheme when the API is added to the scheme?

This was just something I noticed while trying to move things to openshift/api. I saw us registering these generically-named objects, and noticed that some where missing (Ovirt and OpenStack). So since the code was working without needing to register these types, why register them (to be clear, I'm genuinely asking this question)?

Perhaps I'm missing the bigger picture, but what does registering types that we cannot directly query out of etcd get us feature-wise (since we have to go through the codec to unpack these provider types)?

@joelddiaz
Copy link
Contributor Author

After some more discussion this approach would be considered non-idiomatic and may end up surprising someone consuming CCO's API objects. Closing.

@joelddiaz
Copy link
Contributor Author

@staebler @dgoodwin let's revisit this PR. It looks like from the comments at openshift/api#683 (comment) , that there is some surprise that we're registering some of our embedded types. And it would make for a cleaner migration into openshift/api if we resolved this in CCO before attempting to migrate our types to openshift/api.

@joelddiaz
Copy link
Contributor Author

/retest

@joelddiaz
Copy link
Contributor Author

lets look into removing these from pkg/api (so they aren't part of what is submitted to openshift/api).

@joelddiaz joelddiaz closed this Aug 18, 2020
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants