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

feat(crd-generator): Add support for validation rules #5788

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

baloo42
Copy link
Contributor

@baloo42 baloo42 commented Mar 11, 2024

Description

Adds support for Kubernetes validation rules to CRDGenerator:

  • Validation rules can be added to attributes, accessor methods or classes
  • Validation rules can be added to the CustomResource class itself (=the root)
  • Validation rules from inherited classes will be discovered

Refers to #4367

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

TODO's

  • Fix JavaDoc
  • Add more unit tests

Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this improvement, looks good to me, but I should ask to test the content of the newly generated fields 🙏

Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

LGTM, @baloo42 are you planning to iterate more on this?
I think that's good to go 😄 feel free to remove it from "Draft"

- Allow to define validation rules on classes
- Combine validation rules from attributes and accessor methods
- Combine validation rules from inherited classes and attributes/accessor methods
@baloo42
Copy link
Contributor Author

baloo42 commented Mar 19, 2024

@andreaTP Now I'm ready. I added support for some advanced use cases.
Please have a look at the added example and the expected result.

Now the only feature which is still missing is support for meta-annotations. But that's missing for the other annotations, too. Have you discussed the support of meta-annotations in the CRDGenerator in the past?

@baloo42 baloo42 marked this pull request as ready for review March 19, 2024 02:02
Copy link

sonarcloud bot commented Mar 19, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
9.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

A couple of minor comments that should slightly simplify the PR, but looks very promising!

@andreaTP
Copy link
Member

Now the only feature which is still missing is support for meta-annotations. But that's missing for the other annotations, too. Have you discussed the support of meta-annotations in the CRDGenerator in the past?

not sure I'm getting this one, maybe this is related: #3768

@baloo42
Copy link
Contributor Author

baloo42 commented Mar 19, 2024

Now the only feature which is still missing is support for meta-annotations. But that's missing for the other annotations, too. Have you discussed the support of meta-annotations in the CRDGenerator in the past?

not sure I'm getting this one, maybe this is related: #3768

#3768 is something different. (But should be implemented too in my opinion)

With "meta-annotations" support I mean that a user is able to combine our existing annotations to new annotations like described in the following article with JUnit annotations:
https://dzone.com/articles/what-are-meta-annotations-in-java

Spring, for example has also support for Meta-Annotations/Composed Annotations:
https://docs.spring.io/spring-framework/reference/core/beans/classpath-scanning.html#beans-meta-annotations

Is this something we want to support in the future? (Is there an easy and performant way to implement it with sundrio?)

@andreaTP
Copy link
Member

With "meta-annotations" support I mean that a user is able to combine our existing annotations to new annotations

That's interesting, and it has not been discussed previously.
I encourage you to open a separate issue, and I would love to see a strawman on how we can leverage this feature.
If I understand the problem space correctly with meta-annotations we should be able to combine, for example, io.fabric8.generator.annotation.Max with org.hibernate.validator.Max and that would be a great use-case (also for the java-generator)

Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

This PR LGTM, thanks again for all the help @baloo42 ! Appreciated 🙏

facade.nullable,
facade.required,
facade.preserveUnknownFields);

addProperty(possiblyRenamedProperty, builder, possiblyUpdatedSchema, options);
}

List<KubernetesValidationRule> validationRules = Stream
.concat(definition.getAnnotations().stream(), definition.getExtendsList().stream()
.flatMap(classRef -> GetDefinition.of(classRef).getAnnotations().stream()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed here a way to resolve annotations for inherited classes. Is this the right way to implement it with sundrio?

GetDefinition.of(classRef)

@baloo42
Copy link
Contributor Author

baloo42 commented Mar 20, 2024

With "meta-annotations" support I mean that a user is able to combine our existing annotations to new annotations

That's interesting, and it has not been discussed previously. I encourage you to open a separate issue, and I would love to see a strawman on how we can leverage this feature. If I understand the problem space correctly with meta-annotations we should be able to combine, for example, io.fabric8.generator.annotation.Max with org.hibernate.validator.Max and that would be a great use-case (also for the java-generator)

The issue has been created: #5823
Let's discuss some use cases in this issue.

(Some use-cases can be useful and can be implemented, some won't work)

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa merged commit 3ae4f9d into fabric8io:main Mar 25, 2024
16 of 19 checks passed
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.

4 participants