Tekton Backwards Compatibility & Code Sharing #2160
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
- extract go generation logic to shared package - extract common model-annotator - support for omitempty in the shared generator and model-annotator logic - added required indentation style for Makefile in the .editorconfig [tekton-annotator] - replace tekton-annotator with shared model-annotator [tekton-generator] - generate separate tekton schema for versions using different go source dependency version - use shared go generation logic for tekton generators - use go vendor directory for tekton generators - skip building the generator binary for tekton, instead only run the code [tekton-model] - split tekton model by version into v1alpha1 and v1beta1 - remove the generated *Schema class for tekton
Fabian-K
force-pushed
the
separate-model-schema
branch
from
428f8b4
to
d67bd94
Compare
|
SonarCloud Quality Gate failed.
|
There was a problem hiding this comment.
Thanks for the PR. I have some minor comments.
bdw, tekton-v1beta1 model jar also contains classes with v1alpha1 version. Is it okay?
~/go/src/github.com/fabric8io/kubernetes-client/extensions/tekton/model-v1beta1 : $ jar -tf target/tekton-model-v1beta1-4.9-SNAPSHOT.jar | grep v1alpha1
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/DoneablePipelineResourceSpec$1.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/DoneablePipelineResourceSpec.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/DoneableResourceParam$1.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/DoneableResourceParam.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/DoneableSecretParam$1.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/DoneableSecretParam.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpec.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecBuilder.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecFluent$ParamsNested.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecFluent$SecretsNested.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecFluent.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecFluentImpl$ParamsNestedImpl.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecFluentImpl$SecretsNestedImpl.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/PipelineResourceSpecFluentImpl.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/ResourceParam.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/ResourceParamBuilder.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/ResourceParamFluent.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/ResourceParamFluentImpl.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/SecretParam.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/SecretParamBuilder.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/SecretParamFluent.class
io/fabric8/tekton/v1beta1/internal/pipeline/pkg/apis/resource/v1alpha1/SecretParamFluentImpl.class
~/go/src/github.com/fabric8io/kubernetes-client/extensions/tekton/model-v1beta1 : $
|
|
||
| replace github.com/fabric8io/kubernetes-client/generator v0.0.0 => ./../../../generator | ||
|
|
||
| go 1.13 |
There was a problem hiding this comment.
I remember you said v1alpha1 needed to be built under go 1.13, Does v1beta1 requires it too?
There was a problem hiding this comment.
Öhm.. did I? To be honest I can´t remember and currently also can´t think of a reason for requiring a specific go version
| @@ -0,0 +1,3 @@ | |||
| module github.com/fabric8io/kubernetes-client/generator | |||
There was a problem hiding this comment.
Umm, the same goes for having a global generator. I would rather have it inside extensions rather than at the root of kubernetes client
There was a problem hiding this comment.
Same comment as below :)
| <version>4.9-SNAPSHOT</version> | ||
| </parent> | ||
|
|
||
| <artifactId>tekton-model-annotator</artifactId> | ||
| <artifactId>model-annotator</artifactId> |
There was a problem hiding this comment.
Umm, not sure whether it's a good idea to have global model-annotator. I would rather keep an annotator specific to all extensions or maybe to tekton only as it was earlier
There was a problem hiding this comment.
I think that with a good model-annotator there is no reason to have specific (currently just copy&pasted implementations) for the modules (tekon, knative, ...). I know that this implementation at the current point can at least be used for tekton and knative and maybe even further (including default k8s).
But I´m open to moving it somewhere else :)
| "reflect" | ||
| ) | ||
|
|
||
| func main() { | ||
|
|
||
| // continue from here -> somehow the ArrayOrString is fucked up | ||
| // and local changes of common part are not considered ?! | ||
|
|
There was a problem hiding this comment.
ups, that comment should not be in there 🙈 - it was only temporary as a reminder for myself and is resolved. I´ll remove it.
Yes, that is intended. When moving from v1alpha1 to v1beta1 they did not a full copy of all classes/structs. Instread they just cross-reference parts that did not change from alpha -> beta. To make sure that there is no clash, the generator prefixes them using the package (io.fabric8.tekton.v1beta1.internal for beta and io.fabric8.tekton.v1alpha1.internal for alpha) |
Fabian-K
force-pushed
the
separate-model-schema
branch
from
d67bd94
to
24720d5
Compare
|
Currently we are supporting multiple versions for the same API in the same Java module.
Given this statement I understand that the problem is that our generator generates an incorrect class with reference to v1beta1 TaskSpec fields and that in scope of this PR we are fixing that at the generator level by providing some overrides specific to each of the supported API versions. Thus creating separate modules with independent generators for each version. Is this correct? If the only reason to provide an extra module with the extra-overhead it generated in order for the complete project to get compiled, I would suggest to take a different approach such as persisting the generated faulty class and do the fixes there. Same as we are doing in so many other cases (+ add an extra-line in the pom to remove the generated class to avoid collisions). I understand that you've put a lot of work into this ❤️, but if I'm understanding it correctly (which probably I'm not) I see this like a huge overkill to accomplish just the replacement of the v1beta1 TaskSpec fields wrongly inlined in the schema. Am I missing something else? Is the alternative approach something that would work for you? |
|
Thx @manusa for your comments! Let me put this first: if this gets discarded that is totally fine for me! On a high level, my goal is backward compatibility: I think that it would be a benefit if the client can be used for alpha as well as for beta resources. The way how this is achieved is secondary. One difference between the examples (HPA <-> Task) is that in the go source code for HPA there is no cross-referencing, instead, they duplicated the struct. This is very convenient for the generator, because it will by default create two classes containing only the fields that are valid ('behavior' will only be present in the v2beta2) In contrast to that, tekton used a different approach. To quickly summarize the changes between alpha and beta: Alpha had 'inputs' and 'outputs' in its spec. Beta removed those fields, and instead added 'resources' and 'params'. The way this is reflected in the go definition is different compared to HPA. Instead of duplicating the Task, they embedd the beta task inside the alpha Task and append the alpha-only fields ('inputs' and 'outputs'). When using only one module with the generator targeting tekton master revision the TaskSpec class for beta will be perfect (it will contain only fields valid for beta). The TaskSpec class for alpha however will also contain the 'resources' and 'params' field which is only valid for beta (due to the go definition embedding). The fields must not be present in the json (for alpha) send to k8s as the admission webhook will reject unknown fields ('Message: admission webhook "webhook.tekton.dev" denied the request: mutation failed: cannot decode incoming new object: json: unknown field "params"'). One way to fix this is to respect omitempty (which is btw also included in this PR) so technically you would be able to create alpha tasks but only as long as no entries are added for the beta only fields. Of course this has the downside that the developer needs to know this - the java api does not prevent you from doing this. To address this, we could also enhance the generator to skip certain fields - I don´t know the effort estimate for this but i fear this leads to quite some "special case handling" which might be difficult to maintain on the long term. With the approach implemented in this PR there is only one generator logic which does not require any special cases. Because of one module targeting tekton alpha revision and the other targeting tekton beta revision the java classes are accurate (they only contain valid fields for the version). I could imagine that this results in less maintenance effort on the long term - but I don´t have hard facts for it :D |
|
Ok, so this is more or less what I understood. What I'm actually trying to say here is that you can actually persist the class that is generated in The current PR puts a lot of effort in creating a specific generator for a version of the API which in the end only accomplishes the same result that we can achieve if we persist the conveniently modified generated sources. e.g. package io.fabric8.tekton.pipeline.v1alpha1;
/* ... */
public class TaskSpec implements KubernetesResource {
/* ... Other fields and properties ... */
// Or maybe even completely remove these unsupported fields ?
@JsonProperty("params")
@JsonInclude(JsonInclude.Include.NON_EMPTY)
private List<ParamSpec> params = new ArrayList<ParamSpec>();
@JsonProperty("resources")
@JsonInclude(JsonInclude.Include.NON_NULL)
private TaskResources resources;
/* ... Other fields and properties and methods ... */
}My opinion would be different if the current global model-generator was modified to add the convenient Jackson annotations to the optional fields, because then the solution will be consistent for all modules and we could still keep the both API versions in the same Java module. But this would create other risks such as breaking other APIs. There's also another dangerous option to modify the global serializer and set the serializing options to include NON_EMPTY, but this is even more dangerous. |
|
Again, thanks manusa:) I see, let me comment on that: I disagree that there is currently a global model-generator. This might be true for k8s core, but at least for the extensions tekton & knative, looks like that the generator code (pkg/schemagen) was copy and pasted. I don´t like this code duplication. In addition (and IMO more important), the code does not "natively" support the handling of embedded structs in go. Instead, a workaround was baked into it (e.g. https://github.com/fabric8io/kubernetes-client/pull/2033/files) and this will need to be enhanced for other CRDs that might come in the future (other extensions, new core Kubernetes CRDs, ...). This was the main motivation for me to rewrite the generator logic (#2134) which directly supports embedded structs without additional configuration/workaround. At the first step, I only applied it to tekton and this was working well. As a second step, I checked with #2157 if it can be applied to knative as well. Looks like yes so I´m confident (although I don´t know it for sure) that it can also be applied to k8s core. Of course the native handling of go structs could have also been added to the existing k8s core generator but I found it difficult to find the right point in the code to add this. Also (and this is of course only IMO, others may think the other way around) I prefer the "configuration part" (what remains as cmd/generate/generate.go for the individual module) of the rewrite. Also looking at the possibility to exclude certain classes from the generation (ArrayOrString as an example) which makes the deletion via pom.xml obsolete. I agree that copying the class and manually modifying it is another way to tackle the problem. I personally don´t like it as this might result in quite some lines of java code that manually need to be maintained. (I ❤️ about this project that it automates/generates major parts!) I agree that modifying the global serializer is not a good idea :) |
Yes you're completely right there. What I meant was to "merge" both/all generators and keep just one with logic that works for all APIs (not only Tekton or Knative extensions). It's perfectly OK to achieve this in several phases. As you said, generators seem to be copy+pasted and adapted which makes maintenance quite difficult. But in the end, the resulting Go generator should live under the kubernetes-model Java module (don't know how that would work in Go), rest of extensions should be using that one.
Why is it necessary to keep APIs in separate java modules and specific generator invocations then? Can't we just tweak the generator so that it does things for specific API versions? |
|
Fully agree on the point "there should be only one generator" and also the way forward to achieving this (step by step). I´ll leave it up to you to either use the rewrite proposed by me or what is currently in kubernetes-model (and add e.g. the embedded struct handling there). For my rewrite, I know that this works well for tekton & knative. I did not check k8s core so far but would definitely try it out! Of corse moving the rewrite to kubernetes-model is no issue! Now with respect to the one module or multipele module question: I don´t think that compilation time is vastly different with multiple modules. It´s not that the alpha module compiles all classes (alpha & beta) and the beta module does the same again (alpha & beta). Instead, by only passing the alpha CRDs to the alpha module via configuration (https://github.com/Fabian-K/kubernetes-client/blob/24720d570b0788e182cb29e7c0e2757ffdca2731/extensions/tekton/generator-v1alpha1/cmd/generate/generate.go#L29-L41) and the beta CRDs to the beta module (https://github.com/Fabian-K/kubernetes-client/blob/separate-model-schema/extensions/tekton/generator-v1beta1/cmd/generate/generate.go#L28-L37) this should not be as bad as it might initially sound. Also I really like your idea in #2119 (comment) to turn it around and include the generated classes (after sundrio,.. etc processing) in the repo and only executing the time consuming processing when something changes. This would also make the (maybe higher) compilation time less important, right? Maybe this is worth a try! What is your opinion on the following: trying to apply the generator rewrite to the entire project step by step (maybe tekton -> knative -> k8s core). For tekton only, try the multiple module aproach (as it is already implemented). If that works well, expand it to other modules that require it. If not, aim for more configuration? |
|
Just checked build times from latest CI runs (should have probably done this first, sorry), so apparently it takes more or less the same time to build separate modules and a unique module:
IThere aren't so many classes, so I guess sundrio performance doesn't get affected. For the model tweak I was talking more of a way to customize the builder on a per-class basis (I recall doing something similar in the main module builder to be able to rename classes). I was not referring to detect if we are processing some class or some other and act accordingly, just the opposite (add more configuration). Anyway, I haven't dig into this and as you say would require substantial additional effort.
I'm all in favor. Again, sorry for being so picky, but when I see changes that may imply any additional maintenance "cost" I get a little paranoid. After a deeper review I think that after merging this we'll even get a better maintenance experience. |
|
Oh I don´t think you are picky at all. Instead, I really appreciate your comments and the time you spend for the review :) |
|
[merge] |
This PR implements the ideas outlined in #2159
It aims to make the tekton client backward compatible. You can use
client.v1alpha1().xxxtargeting tekton-pipelines <= v0.10.x (supporting only v1alpha1) but also >= v0.11.x (supporting both v1alpha1 and v1beta1).As a result of using schemas based on v0.10.x for v1alpha1 and v0.11.x for v1beta1, the Java APIs contain only the fields supported by the version (e.g.
inputsfor Task Spec v1alpha1 and notparamswhich is only included in v1beta1).As always, I´m looking forward to your feedback!
Best regards,
Fabian
A detailed list of changes:
[common]
[tekton-annotator]
[tekton-generator]
[tekton-model]