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

chore(crd-generator): Add approval tests for crd-generator api-v2 #5978

Closed

Conversation

baloo42
Copy link
Contributor

@baloo42 baloo42 commented May 3, 2024

Description

Add approval tests for crd-generator api-v2.

Refers to #5850

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.)

@baloo42
Copy link
Contributor Author

baloo42 commented May 3, 2024

I have copied within the first commit the approval tests from the api module without changes.
The second commit contains the required adjustments, so that the changes are easy comparable:
a7eda34...baloo42:kubernetes-client:crd-generator-v2-approval-tests

@manusa
Copy link
Member

manusa commented May 3, 2024

I'd actually like to move this outside so that the same tests can be shared by the two modules.
This will also help out to clarify what are the breakages and how users might migrate to the new generator.

Copy link

sonarqubecloud bot commented May 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@baloo42
Copy link
Contributor Author

baloo42 commented May 3, 2024

I'd actually like to move this outside so that the same tests can be shared by the two modules.
This will also help out to clarify what are the breakages and how users might migrate to the new generator.

There are some differences between the used source files and the output:

it works at the property level, rather than the field level, so some crds may have fields removed - but those weren't actually being used. This is also the reason test code made fields public or used the lombok Data annotation - Jackson doesn't consider a private field without accessors serializable.

  • v2 serializes all strings with quotes

In conclusion, neither the source files nor the approval files can be used for both without further adjustments.

Do you have an idea how to overcome this problem?
The source files can be shared easily (if we adjust the corresponding approval files) but the approval files itself?

@manusa
Copy link
Member

manusa commented May 3, 2024

I want to spend some time on this (probably on Monday).

Will probably require some adjustments to the test files (and maybe the serialization settings for v2).

My initial goal would be to not duplicate the tests preserving as much as possible so that we can get a clear idea of what's changing.

@shawkins
Copy link
Contributor

shawkins commented May 3, 2024

v2 works at the property level

I see this as a hard requirement as it's also the correct behavior. I don't think we want to support a mode of the CRD generator where it's operating on record / struct like beans - that aren't actually usable by the fabric8 client. So the existing source files should be adjusted to use the Data annotation, the fields made public, etc.

Will probably require some adjustments to the test files (and maybe the serialization settings for v2).

It would be easier to change the serialization of v1 to match v2. Changing the serialization for v2 will mean adding serialization options to the KuberentesSerializer, which so far we have avoided.

@baloo42
Copy link
Contributor Author

baloo42 commented May 3, 2024

I want to spend some time on this (probably on Monday).

No hurries, I just wanted to start with the discussion on the strategy (there are many ways, so I started with simplest way).

Please let me know, if I can help and don't hesitate to close the PR if it's easier for 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.

3 participants