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

Initial prop for custom resource definitions #722

Merged
merged 1 commit into from
Feb 6, 2018

Conversation

shawn-hurley
Copy link
Contributor

Describe what this PR does and why we need it:
Proposal for using custom resource definitions.

@shawn-hurley shawn-hurley added proposal needs-review 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 labels Feb 2, 2018
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 2, 2018
shawn-hurley added a commit to shawn-hurley/ansible-service-broker that referenced this pull request Feb 2, 2018
@shawn-hurley shawn-hurley mentioned this pull request Feb 2, 2018
Copy link
Contributor

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

Should we generate our own client or write our own. This is a nice overview of client generation. This is an example of client generation. This is an example of creating our own.

I would prefer to avoid code generation if we can help it.

We should discuss where this client lives. Should we create a new repo for this client so that other users could interact with our resources? This may eventually be a nice to have but could also cause headaches.

I'm not sure we should publicize the fact that these are accessible to normal users. The broker makes some assumptions that these are internal use only. It's never been designed for this.

Proposal though, +1. Great work @shawn-hurley.

@maleck13
Copy link
Contributor

maleck13 commented Feb 2, 2018

@shawn-hurley @eriknelson We use client generation for a CRD that we use with the CLI we are creating. In my experience it saves quite a lot of time (although the initial setup is a cost) as it generates a full client with a very similar API to the kubernetes go-client. It also generates all the fakes that you need to allow simple testing when using the client.
If you are interested here is our definition:
https://github.com/aerogear/mobile-cli/tree/master/pkg/apis/mobile
The client it generates lives here:
https://github.com/aerogear/mobile-cli/tree/master/pkg/client
And here is an example of the fakes that allow unit tests
https://github.com/aerogear/mobile-cli/blob/master/pkg/cmd/clients_test.go#L47
Finally here is the make target to generate the clients
https://github.com/aerogear/mobile-cli/blob/master/scripts/generate.sh

Edit wrong link

@shawn-hurley
Copy link
Contributor Author

I am adding my two sense here because I realize I did not advocate for one way or the other in the proposal.

I agree that for the first pass that we need to keep these for internal use only. I think soon after we may have to start dealing with cases where things change underneath us.

The client I think is going to start easy with basic CRUD and get much more involved. I wouldn't mind taking this approach first and reevaluating latter if the maintenance of it becomes a pain.

I think that in the end, we will not want to create and manage listers/watchers and the rest when it comes to the client. A full client is actually pretty involved, and I think there might be a case for making a kube client look like other kube clients.

No matter where the client lives I think it makes sense to keep it isolated so that if we need to it should be easily ripped out.

jmrodri pushed a commit that referenced this pull request Feb 6, 2018
@eriknelson eriknelson merged commit c75edfa into openshift:master Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 needs-review proposal size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants