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

MGDSTRM-10781 adding a jsonproperty as a hint to the crd generator #872

Merged
merged 3 commits into from
Mar 2, 2023

Conversation

shawkins
Copy link
Contributor

While the builder and jackson logic are fine dealing with the lombok accessor names, the crd generator is not. A jsonproperty is needed for the private property to correctly appear in the crd.

@github-actions github-actions bot added the api changes related to api label Feb 27, 2023
@MikeEdgar MikeEdgar added this to the 0.34.0 milestone Feb 27, 2023
Copy link
Contributor

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

LGTM. There are two src/test/resources CRDs in the new bundle module that have the property with the _. Would you mind to adjust them here as well?

Copy link
Contributor

@biswassri biswassri left a comment

Choose a reason for hiding this comment

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

lgtm

@shawkins
Copy link
Contributor Author

LGTM. There are two src/test/resources CRDs in the new bundle module that have the property with the _. Would you mind to adjust them here as well?

@MikeEdgar if we want to keep those in-sync, then how about driving the test from the artifact rather than the resources? I've added that as a speculative commit. It does appear like there are stable ordering issues with the csv and this does mean we'll have to update the expected every time there's a change - but it will also serve as a cross check.

If you want to keep the test loosely coupled, that's fine too.

@shawkins
Copy link
Contributor Author

It looks like the stable ordering concerns may be too pervasive: https://github.com/bf2fc6cc711aee1a0c2a/kas-fleetshard/actions/runs/4298073875/jobs/7491779314#step:5:6804

@MikeEdgar
Copy link
Contributor

It looks like the stable ordering concerns may be too pervasive: https://github.com/bf2fc6cc711aee1a0c2a/kas-fleetshard/actions/runs/4298073875/jobs/7491779314#step:5:6804

What about using something like jsonassert? That can do comparisons non-strict array ordering.

@shawkins
Copy link
Contributor Author

shawkins commented Mar 1, 2023

What about using something like jsonassert? That can do comparisons non-strict array ordering.

Unfortunately ordering matters for some of the arrays, so I wasn't sure if we wanted to go that far. We should be somewhat confident that the assembler logic is not introducing the order changes - are you saying you're good checking without strict ordering?

I'll also discuss / open an upstream issue if needed with the operator folks to improve stablity - unfortunately it looks like both operator sdk and quarkus introduce unpredictable orderings.

@MikeEdgar
Copy link
Contributor

Unfortunately ordering matters for some of the arrays, so I wasn't sure if we wanted to go that far. We should be somewhat confident that the assembler logic is not introducing the order changes - are you saying you're good checking without strict ordering?

I expect the assembler output to be stable between runs. Aside from convenience, I'm wondering if we need to be concerned about array order for these resources when considering correctness. Looking through the CSV, I think any ordering of the arrays should be equivalent from the OLM/K8s perspective.

Upstream array order stability would be nice too 😄

@shawkins
Copy link
Contributor Author

shawkins commented Mar 1, 2023

Looking through the CSV, I think any ordering of the arrays should be equivalent from the OLM/K8s perspective

Nearly. Things like container ordering matters to the notion of what the default container is.

I'll update the pr ignore ordering for now.

Upstream array order stability would be nice too

operator-framework/java-operator-sdk#1791

@shawkins
Copy link
Contributor Author

shawkins commented Mar 1, 2023

@MikeEdgar inexplicably the inclusion of the jsonassert test dependency in the bundle project seems to be causing build failures in other modules. Not quite sure what is going on yet.

@MikeEdgar
Copy link
Contributor

MikeEdgar commented Mar 1, 2023

@shawkins , I've seen this one a lot lately - There was a timeout in the fork. It seems to happen when the tests run longer than 3 minutes. I've bumped basepom.test.timeout to 240s/4m in #873, you can do the same here and whichever merges second can deal with the conflict.

@shawkins
Copy link
Contributor Author

shawkins commented Mar 2, 2023

@MikeEdgar marked this for a re-review given the additional changes.

Checking back prior to the issues yesterday and we were very close to the timeout https://github.com/bf2fc6cc711aee1a0c2a/kas-fleetshard/actions/runs/4287405806/jobs/7468221679#step:5:6849

I was also hitting 2 other build issues though that ended up being ephemeral and not reproducible locally. In any case it seems to be fine now.

Copy link
Contributor

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@shawkins shawkins merged commit 6078800 into bf2fc6cc711aee1a0c2a:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api changes related to api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants