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

Generate models #55

Closed
wants to merge 25 commits into from
Closed

Generate models #55

wants to merge 25 commits into from

Conversation

mortenlj
Copy link
Member

This is a big PR, but you can ignore all the files under k8s/models/v1_*, as they are generated. You may want to take a look at a couple just to see how they end up, but they should be fairly straight forward. Most commits are somewhat contained, and can be reviewed independently, although some things naturally change by the time you get to the end.

The gist of the PR is in model.jinja2 and generator.py. The code is not really that pretty, and doesn't have any tests. As far as I can tell, it generates most things as it should. I have had to exclude some things that we don't have a good way of solving, or that simply doesn't fit with our current API.

During the work, I have removed all the old manually written models, and also removed the OnceField, which we only used in one place, and I couldn't find any sensible way to extract from the spec when to use it. I also don't think it is needed.

Whenever the specs have moved a model, I have just included it at the new location, and not kept any kind of alias or similar around. In theory we could have done that, but I don't see the point to be honest.

I think the next release after this is merged should be 1.0. We may want to write a short migration guide, as all the models you used to know have been moved.

There are also a few more abstract questions that should be answered, which may change things:

  • I have generated and commited all the models. Should we instead run generation when building?
  • I have generated models for all versions from 1.6 to 1.13, putting them in separate packages. Should we instead just pick one? ..or two? If so, which?

@mortenlj mortenlj self-assigned this Jan 10, 2019
@fiaas fiaas deleted a comment Jan 11, 2019
@fiaas fiaas deleted a comment Jan 14, 2019
@mortenlj mortenlj mentioned this pull request Jan 16, 2019
@mortenlj mortenlj requested a review from a team September 4, 2019 07:32
@birgirst
Copy link
Contributor

birgirst commented Sep 5, 2019

Should we try to rebase this one and get it merged?

@mortenlj
Copy link
Member Author

mortenlj commented Sep 5, 2019

That would be ideal, but first we need to discuss the questions and come to conclusions, so we know what we want to do.

@oyvindio
Copy link
Member

The official python client for Kubernetes seems to target one Kubernetes version for each major version: https://github.com/kubernetes-client/python#compatibility. Maybe this is the simplest thing to maintain? Most changes in the Kubernetes API seem to be additive, which makes them backwards compatible at least across some number of versions.

One benefit of generating a package for each Kubernetes version is that you can have code targeting different, possibly incompatible Kubernetes versions while depending on a single version of the k8s library. However, this isn't a problem I've personally seen while working on FIAAS/integrating with Kubernetes for the last years, so maybe it is an unlikely use case. An obvious drawback of the one package per version approach is that there will be a lot of code after a while, and a lot of it will be more or less the same across different versions.

I think both approaches are big improvements over the current situation though

@mortenlj
Copy link
Member Author

The approach here isn't really optimal and also starting to get outdated. With latest changes to CRDs that add fields for schemas, I have a seed of an idea about doing something more generic or reusable that would allow to use the same tooling for CRDs as for the "normal" Kubernetes types. So I'm closing this PR, because it's not going anywhere anyway.

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.

None yet

3 participants