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

CRDInfo doesn't properly support multiple CR versions #3371

Closed
metacosm opened this issue Aug 2, 2021 · 0 comments · Fixed by #3367
Closed

CRDInfo doesn't properly support multiple CR versions #3371

metacosm opened this issue Aug 2, 2021 · 0 comments · Fixed by #3367
Assignees
Labels
component/crd-generator Related to the CRD generator
Milestone

Comments

@metacosm
Copy link
Collaborator

metacosm commented Aug 2, 2021

If you have multiple versions of a given CR, represented by different classes, one CRD is generated per CRD spec version but this CRD contains multiple versions. In this situation, the set of returned dependent classes is incorrect as the ClassDependenciesVisitor only records one CR class name per CRD name.

@metacosm metacosm self-assigned this Aug 2, 2021
@metacosm metacosm added this to the 5.7.0 milestone Aug 2, 2021
metacosm added a commit that referenced this issue Aug 2, 2021
Since several classes can take part in the CRD generation, one per CR
version, we need to record all associated class names so that dependents
for each of these classes can then be recorded during the generation
traversal process. We also need to aggregate all the dependents for all
the classes associated with each CR version when we want to retrieve the
transitive closure of dependents affecting the generation of a given
CRD. Added tests.

A downside of this is the fact that we needed to revert the publication
of additional information on CRDInfo because this was actually incorrect
since the mapping of CRDInfo to one CR class / spec / status is actually
invalid. To properly provide that information, we would need to create a
more elaborate CRDGenerationInfo structure which would go deeper: this
is currently a 2-level Map, indexed first by CRD name, then CRD spec
version. We would need a third level to also record the CR version so
that we have a unique mapping for between a [CRD name, CRD spec version,
CR version] key and related CR class information. As this would be
complex to implement, we're deferring this until a need actually arises.

Fixes #3371
@manusa manusa added the component/crd-generator Related to the CRD generator label Aug 3, 2021
manusa pushed a commit that referenced this issue Aug 4, 2021
Since several classes can take part in the CRD generation, one per CR
version, we need to record all associated class names so that dependents
for each of these classes can then be recorded during the generation
traversal process. We also need to aggregate all the dependents for all
the classes associated with each CR version when we want to retrieve the
transitive closure of dependents affecting the generation of a given
CRD. Added tests.

A downside of this is the fact that we needed to revert the publication
of additional information on CRDInfo because this was actually incorrect
since the mapping of CRDInfo to one CR class / spec / status is actually
invalid. To properly provide that information, we would need to create a
more elaborate CRDGenerationInfo structure which would go deeper: this
is currently a 2-level Map, indexed first by CRD name, then CRD spec
version. We would need a third level to also record the CR version so
that we have a unique mapping for between a [CRD name, CRD spec version,
CR version] key and related CR class information. As this would be
complex to implement, we're deferring this until a need actually arises.

Fixes #3371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/crd-generator Related to the CRD generator
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants