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

improve: simpler api for adding additional CRD file #2574

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Nov 14, 2024

Signed-off-by: Attila Mészáros a_meszaros@apple.com

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri linked an issue Nov 14, 2024 that may be closed by this pull request
@csviri csviri marked this pull request as draft November 14, 2024 22:33
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2024
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri marked this pull request as ready for review November 14, 2024 22:36
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2024
@@ -28,7 +28,7 @@ public class CRDMappingInTestExtensionIT {
LocallyRunOperatorExtension operator =
LocallyRunOperatorExtension.builder()
.withReconciler(new TestReconciler())
.withCRDMapping("tests.crd.example", "src/test/crd/test.crd")
.withAdditionalCRD("src/test/resources/crd/test.crd")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was explicitly not put in resources to check that the file could be accessed from a "random" part of the file system, not just from the classpath.

Copy link
Collaborator Author

@csviri csviri Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, isn't it trivial? I would rather stick with this, but no strong opinion, if you insist I can change it back.

@@ -138,6 +144,17 @@ private static void applyCrd(InputStream is, String path, KubernetesClient clien
}
}

public static CustomResourceDefinition parseCrd(String path, KubernetesClient client) {
try (InputStream is = new FileInputStream(path)) {
return (CustomResourceDefinition) client.load(new ByteArrayInputStream(is.readAllBytes()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should handle better weird cases: if there are more than just one resource in the file, if the CRD is not a v1 spec, if the resource in the file is not a CRD are the most obvious cases where this might fail.

Copy link
Collaborator Author

@csviri csviri Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update it to handle more CRD in file. I don't think we should support other CRD version that v1, note that this is not the resource definition version inside, CRD api version itself is stable for very long time. (In actual kubernetes release oldel versions are not even supported)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but the APT version of the CRD generator generates v1beta1 as well (and in fact, you can see that these versions are generated in our tests). It would actually be useful not to generate the v1beta1 version to save some time on the tests since we have a lot of CRDs to generate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree, we should fix that, thus not generate those anymore.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri merged commit 134d5c0 into main Nov 19, 2024
20 checks passed
@csviri csviri deleted the crd-path-simpler-api branch November 19, 2024 16:03
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.

Confusing hard-coded v1 expectation for CRD Yamls
2 participants