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

Use resource pattern resolver to look up CRDs from classpath #196

Conversation

ajalab
Copy link
Contributor

@ajalab ajalab commented Dec 27, 2024

fixes #195.

This PR updates DefaultCRDApplier to use PathMatchingResourcePatternResolver to look up CRD manifests from Java classpath resources in arbitrary formats, including JARs pulled from a dependency.


private Resource[] findResources() {
final var resourceResolver = new PathMatchingResourcePatternResolver();
final var resourceLocationPattern = crdPath + '*' + crdSuffix;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users may find it more useful if they can specify the entire location pattern or even a set of patterns (e.g., META-INF/fabric8/*-v1.yml and META-INF/fabric8/*-v2.yml). This is a breaking change and outside the scope of the fix, so let me leave it for now.

Copy link
Contributor

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM

@csviri
Copy link
Contributor

csviri commented Jan 6, 2025

Hi @ajalab thank you for PR, could you pls format the code, simply by running mvn clean install, and push it?

thank you!

@ajalab
Copy link
Contributor Author

ajalab commented Jan 7, 2025

Thanks! mvn clean install didn't work in my environment somehow, so I fixed the spotless violation by running spotless:apply task 18b343e.

@ajalab ajalab requested a review from csviri January 7, 2025 03:23
@ajalab
Copy link
Contributor Author

ajalab commented Jan 8, 2025

Umm, according to the test result, there is something wrong in the CRD registration in some tests. Let me work on the fix and let you know after finishing it.

@ajalab ajalab marked this pull request as draft January 8, 2025 01:41
@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 Jan 8, 2025
@ajalab
Copy link
Contributor Author

ajalab commented Jan 8, 2025

It turned out that the test was failed because the previous path concatenation could not handle crdPath without / suffix (e.g., /META-INF/fabric8/deeper) well. I updated to use Paths.get to support both cases c792d95.

@ajalab ajalab marked this pull request as ready for review January 8, 2025 02:12
@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 Jan 8, 2025
@csviri csviri merged commit ae82068 into operator-framework:main Jan 8, 2025
1 check passed
@csviri
Copy link
Contributor

csviri commented Jan 8, 2025

thank you!

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.

DefaultCRDApplier cannot apply CRD resources from JARs in the classpath
2 participants