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

Fix the client set generator #10861

Merged
merged 6 commits into from
Oct 24, 2016

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Sep 9, 2016

  • fix the APIPath to point to /oapi
  • fix the non-namespaced types (projects, ...)
  • regenerate the client sets
  • add simple smoke test for the generated client set

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 13, 2016

[test]

@deads2k PTAL

@deads2k
Copy link
Contributor

deads2k commented Sep 13, 2016

Do they work?

[test]

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 14, 2016

@deads2k guess we need some tests for them... I will work on it today/tomorrow.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 15, 2016

@deads2k now they should work

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 15, 2016

(this have to wait for rebase first)

@mfojtik mfojtik added blocked and removed blocked labels Sep 15, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016
@mfojtik mfojtik changed the title Re-enable the client set generator Fix the client set generator Sep 16, 2016
@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2016

[test]

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2016

@smarterclayton client sets can now be generated again, but I want to add a simple test that proves they work.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016
@deads2k
Copy link
Contributor

deads2k commented Sep 16, 2016

My checklist:

  • add a namespaced resource to the integration test
  • confirm your verify script works (add one if you forgot to do it)
  • followup issue to practice generating a shared clientset with multiple group. We'll need to figure out how to do this for 1.5.

After that, I'd take this as a starting point.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 16, 2016

continuous-integration/openshift-jenkins/test Waiting: Determining build queue position

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2016

@deads2k first and second bullet point should be already addressed here, I will create the follow up

@smarterclayton
Copy link
Contributor

What's the first consumer of these client sets?

On Fri, Sep 16, 2016 at 10:39 AM, Michal Fojtik notifications@github.com
wrote:

@deads2k https://github.com/deads2k first and second bullet point
should be already addressed here, I will create the follow up


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10861 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p_wFmxeBM-mUhvPgy6VdN645O-M2ks5qqqovgaJpZM4J4_fN
.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2016

@deads2k #10956

@smarterclayton an integration test :-) I think we can pick either deployments or builds and try to use the client set there. What are you thoughts?

@deads2k
Copy link
Contributor

deads2k commented Sep 16, 2016

What's the first consumer of these client sets?

Right now, I just want to make sure I have a way to generate them so we can shake out generation problems (one already found).

I see the next steps as:

  1. no more non-generated clients. You add a new API, use a generate clientset.
  2. prove that the generator can create a common clientset based on the FutureGroupName for two groups.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2016

@deads2k do we want to migrate the existing controllers to use client sets at some point? (or maybe start with the CLI first)

@deads2k
Copy link
Contributor

deads2k commented Sep 16, 2016

@deads2k do we want to migrate the existing controllers to use client sets at some point? (or maybe start with the CLI first)

I think I'd try to generate a common clientset with FutureGroupNames first. Getting there may force us to fix generator bugs in 1.5 for when we make the big switch. In addition, it will give you the APIs you need to evaluate whether you can reasonably support new controller against old API servers.

After you've sorted that out, the CLI would probably be my next target.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2016

@deads2k sound good to me.

@smarterclayton
Copy link
Contributor

I'd prefer not to put these in our codebase if we can generate directly to
something like "openshift/client-go" and then back vendor them.

On Fri, Sep 16, 2016 at 12:12 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8926/)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10861 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p9UM-CF_G0nb3FMNdQ-p27ZNaD7pks5qqr_3gaJpZM4J4_fN
.

@liggitt
Copy link
Contributor

liggitt commented Sep 16, 2016

Really? You wouldn't want to go the other direction? Generate here, then extract to client repo on version boundaries?

@smarterclayton
Copy link
Contributor

I'm asking whether it helps or hurts. Should our clients be generated
onto the versions provided by Kube in that client-go? I.e. I don't want
Openshift and Kube client libraries to conflict in dependencies because we
extracted on a slightly different pkg/runtime boundary.

@deads2k
Copy link
Contributor

deads2k commented Sep 19, 2016

I'm asking whether it helps or hurts. Should our clients be generated
onto the versions provided by Kube in that client-go? I.e. I don't want
Openshift and Kube client libraries to conflict in dependencies because we
extracted on a slightly different pkg/runtime boundary.

I hate having problems like this so much. These aren't your grandfather's coding problems. Oh wait, they are.

I think I'd still start with them here while we get the hang of actually generating something useful and deciding how we generate both grouped an ungrouped APIs in a sane inter-operable way. Seems like for the short-term, we'd make sure we're compatible by godep-ing in the upstream client.

@smarterclayton
Copy link
Contributor

We already godep in the upstream client. It's just not split correctly yet.

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 5, 2016

@smarterclayton do we want to split it here in this PR or is this good for the start at least? The current generated client we have in repo is junk atm. This PR at least allows to experiment with it.

I agree with @deads2k that this is a good start and allows to experiment. I'm fine following up with improvements to generator to allow splitting and generating grouped/ungrouped APi clients.

@smarterclayton
Copy link
Contributor

I'm fine with moving forward, but we need to be very clear before we start using them how we transition to external clients.

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 6, 2016

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 663bffe

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9700/)

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 7, 2016

@deads2k any final comments?

@deads2k
Copy link
Contributor

deads2k commented Oct 7, 2016

@deads2k any final comments?

Just a repeat of this:

I think I'd try to generate a common clientset with FutureGroupNames first. Getting there may force us to fix generator bugs in 1.5 for when we make the big switch. In addition, it will give you the APIs you need to evaluate whether you can reasonably support new controller against old API servers.

After you've sorted that out, the CLI would probably be my next target.

If we can find problems with the generator before 1.5 ships, the next rebase will go easier.

[merge]

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 10, 2016

[merge] #11094

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 10, 2016

[merge] #11004

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 12, 2016

flake (#8571)

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 24, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 663bffe

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 24, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9700/) (Base Commit: 9195050) (Image: devenv-rhel7_5236)

@openshift-bot openshift-bot merged commit 28fd83f into openshift:master Oct 24, 2016
@mfojtik mfojtik deleted the reenable-clientset-gen branch September 5, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants