bf2fc6cc711aee1a0c2a / kas-fleetshard

The kas-fleetshard-operator is responsible for provisioning and managing instances of kafka on a cluster. The kas-fleetshard-synchronizer synchronizes the state of a fleet shard with the kas-fleet-manager.
Apache License 2.0
7 stars 20 forks source link

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

Closed shawkins closed 1 year ago

shawkins commented 1 year ago

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.

shawkins commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :smile:

shawkins commented 1 year ago

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

https://github.com/java-operator-sdk/java-operator-sdk/issues/1791

shawkins commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

sonarcloud[bot] commented 1 year ago

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