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): Allow to configure CRDGenerator if running as annotation processor, add approval and compiler tests #5896

Closed
wants to merge 14 commits into from

Conversation

baloo42
Copy link
Contributor

@baloo42 baloo42 commented Apr 15, 2024

Description

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

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 15, 2024

TBD:

  • I used Approval Tests in two cases: In unit tests of the api module and for the compiler tests in the apt module. Please review the setup in detail as I haven't used it before.
  • There are now two modules which test the CustomResourceAnnotationProcessor in an own maven module.
    This approach was really error prone to me and that's the main reason why I wrote the compiler integration tests with google's compile-testing. In my opinion we can remove the test and test-parallel module. What do you think?
  • There are some public static final flags: Are they required? e.g. DESCRIBE_TYPE_DEFS
  • There are now some duplicate unit tests because those cases are also tested by the approval tests. Should I remove them in this PR or do you want me to wait with the deletion?
  • The option parsing is implemented in a very simple way. Is it enough? (If we extract an interface / factories, it would be also easier to test)

Copy link

sonarcloud bot commented Apr 15, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
78.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@baloo42 baloo42 marked this pull request as ready for review April 15, 2024 17:12
@andreaTP
Copy link
Member

I skimmed through it and the changes look to be going in a nice direction, but this is massive, either you manage to chunk it into more digestible bites or it will take some time to properly review.

cc. @shawkins asking for help here 🆘

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 18, 2024

A third of the changes come from the copy of the test module to the new test-parallel module. (It's exactly the same content). So the first question is, do we want to keep both test modules? ...if we have an alternative:

I don't like both because it requires a lot (test case) duplications, but it's some kind of a real end-2-end test and it was there before I started contributing. My suggestion would be to replace those both modules by appropriate "compile-testing" tests in the apt module. With a small abstraction we can build easily tests which run always with parallel mode on and off (and I think we can also run them in parallel via failsafe plugin because they are fully isolated).

Regarding the options:
Implementing more apt processor options requires tests. We would require a lot of modules to test them if we don't change the test strategy. And in the end, I need it for #5794 because here we have to implement a way to configure a ca certificate.

I'm happy to hear any further suggestions or guidence ;)

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 23, 2024

Can probably be closed. See #5885 (comment)

@baloo42 baloo42 marked this pull request as draft April 23, 2024 21:37
@baloo42 baloo42 closed this Apr 30, 2024
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.

None yet

2 participants