Skip to content

Use newest templates for java client #16

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

Closed
wants to merge 2 commits into from

Conversation

mbohlool
Copy link
Contributor

The new swagger-codegen templates for java has a way to create an Operation's Call object. That is required to implement watch. We can remove this when swagger-codegen releases 2.3.0.

depends on #15

@brendandburns
Copy link
Contributor

Do we want to do this or just check in the generated code into the java client tree?

@mbohlool
Copy link
Contributor Author

mbohlool commented Jul 1, 2017 via email

@brendandburns
Copy link
Contributor

Hrm, I thought that a better approach would be to freeze the relevant manually updated files. (I did this with the swagger-codegen-ignore file.

That way manual updates don't get reverted, but we don't have to host the full set of mustache files (which are inevitably going to get out of sync with the main repo...)

@mbohlool
Copy link
Contributor Author

mbohlool commented Jul 6, 2017

The ultimate goal for repo separation is to enable users to provide an OpenAPI spec and get a fully functional client. Our own client should be ultimately the result of this generation process.

The design is to have three repos:

  • base repo: all non-generated files, utils, possibly O(1) generated and modified ones, see below
  • gen repo: Generates a client only depend on base repo given an OpenAPI spec.
  • $lang repo: Generated using gen depends on base using official OpenAPI spec (supported client).

Putting these files in java client makes a dependency from gen repo to lang repo that is against that design.

More of (maybe unrelated) thoughts on the design:

Type of generated files:

1- O(N) Auto generated files where N is number of Operations
2- O(1) Auto generated files (like rest/api client)
3- Manual files such as utils

The ignore flag you mentioned is mostly useful for second option. There we have two options:

A - Change templates for those files to meet our requirements
B - Ignore generating them and use our version. With this option, we need to put them in the same repo as utils.

python client is on option B.

@brendandburns
Copy link
Contributor

brendandburns commented Jul 9, 2017

I think I view it slightly differently. When we pin something because of an upstream fix where we are waiting for the generators to catch up, we are effectively moving something from gen to base, and then when the generators catch up, we effectively move it back from base to gen.

I think vendoring the mustache files is going to be disasterous, since we will have to continually re-vendor in fixes from the upstream generators, etc. And/or there will always be the temptation to fix things in the mustache files and never upstream those changes back into the swagger-codegen repo.

I really don't think that mustache template files belong in our repos.

If we want to make the gen to base and vis-versa movement more explicit via PR that actually moves the files, that's fine with me.

Additionally, I kind of disagree with your plan, I think it is overly complicated for what it is trying to achieve. What is wrong with just having a generated directory inside of a single repository?

@mbohlool
Copy link
Contributor Author

mbohlool commented Jul 10, 2017

I want to break discussion into two section:

A. Should we copy templates from swagger-codegen main repo?
My view on that is not that different than yours. I think we should be very careful about being out of sync with templates there. Even copying the O(1) generated files in core repo can make problems as the O(N) files depend on them. So I think even for O(1) files, we should ultimately use one of the solutions here. These are the use cases I think we can (carefully) copy templates:
A1. If we just want new templates that are not released yet (This is the case here). I won't see any problem here. We can set a bar to this minimum of not changing copied templates and get rid of them when they are released.
A2. What if we want to add some files to templates? Currently Swagger-codegen does not allow extensions to generated files. What if we want a new set of classes? Now that I think about this one, there is a solution without copying templates (it involves running generator twice).
A2.5 (2.5 just because I added it later) What if there is something wrong about templates we want to fix, but they are generic enough (not kubernetes specific)? We should send a PR to the main swagger-codegen repo, get it merged and then follow A1 solution.
A3. What if there is something specific to kubernetes we want to change in templates. e.g., adding watch method to API classes (watchNamespaces... beside listNamespaces...)? Those are not general to be added to upstream. For these ones, we should have a tool in place to update templates before we raise the bar to this level of modification (the tool can check out the original commit, create a diff, check out the newest commit, apply the diff, and send a PR for a human to review). We can even have a bot to publish changes that can be automatically be merged and nagg about changes that cannot. I am not ready to raise the bar to this level until we have those tools in place.

B. If we want to copy templates, where should we put them?
This, I think, is easier to answer and I thought this thread was about that. If we want to copy them, they should be in gen repo (in my opinion). For now if we set bar to minimum, we only copy newest templates (if we need them earlier, like here for watch support). So updating these templates should be as easy as a rebase or remove them when the templates we want are released.

Sorry for long reply. We can continue discussion on another more interactive medium (chat/VC) if you want.

@brendandburns
Copy link
Contributor

@mbohlool I totally understand where you are coming from, but I have a couple of concerns:

  1. I worry that in practice we won't ever come along and actually delete/update the template files.

I'm quite concerned that what will actually happen is that we will copy "latest" but then "latest" will be "released" and eventually "old" and we won't actually update the template files so we'll end up lagging.

  1. I'm worried about having multiple sources of truth. We should never, ever make changes to the template files that are in this repo. I'm worried that simply the act of having them in this repo will encourage people to quickly make changes here instead of upstreaming them to swagger-codegen. Again, if we add some sort of automation to prevent this, that makes me more comfortable.

On the other hand, what is the cost of freezing/updating the generated source code instead of the templates? I don't see any real cost to that (and it's even ok if we move things from the base repo to the client repo when we do.) I don't see any downside to that approach.

But, if you would rather take this approach, as long as we get the right automation in place to deal with these concerns, it seems fine with me.

@mbohlool
Copy link
Contributor Author

On the other hand, what is the cost of freezing/updating the generated source code instead of the templates? I don't see any real cost to that (and it's even ok if we move things from the base repo to the client repo when we do.) I don't see any downside to that approach.

I think I did not understand this approach and how it solves the problem of:

  1. Using newest templates when we need them
  2. Somebody else want to generate their own client

Can you elaborate on that?

@brendandburns
Copy link
Contributor

@mbohlool

I thought about this some more, how is this for a revised compromise:

We don't check in the files here, but we do build a Docker image for generating the client code.

That way, when we want to rev. the templates, we can rev the Docker image to include the newest templates. We can check in the Dockerfile to build that image (which includes checking out/building swagger-gen at the right commit)

That way, we can use the latest templates, enable people to generate from a Docker image, but still not check the templates into our source tree...

Does that work for you?

Thanks

@mbohlool
Copy link
Contributor Author

That sounds like a reasonable compromise to me. I will try to make that docker image. The docker image can also get a specific swagger-codegen branch/tag/sha and build that locally inside the image and use that instead of only templates (there could be logical changes with those new templates, this is safer I guess).

@mbohlool
Copy link
Contributor Author

closing in favor of #20

@mbohlool mbohlool closed this Aug 10, 2017
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.

2 participants