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

Make sure CRDs are in sync with their counterpart Go structs #862

Closed
kensipe opened this issue Sep 27, 2019 · 4 comments · Fixed by #1240
Closed

Make sure CRDs are in sync with their counterpart Go structs #862

kensipe opened this issue Sep 27, 2019 · 4 comments · Fixed by #1240

Comments

@kensipe
Copy link
Member

kensipe commented Sep 27, 2019

What would you like to be added:

We need a test that confirms that the structs used for CRDs are compatible with CRDs. We need to gen out crds and do a round trip check to confirm changes in code are consistent with CRDs.

Why is this needed:

@kensipe kensipe added this to the 0.8.0 milestone Sep 27, 2019
@alenkacz
Copy link
Contributor

I believe this will also be solved with proper e2e tests for some basic scenarios.

@gerred
Copy link
Member

gerred commented Sep 27, 2019

@porridge
Copy link
Member

A little overdue update on my approach:

Currently we need to use our brains to always make sure that the CRDs - for example the OperatorVersion spec needs to match the Go struct fields - otherwise we will either lose data or face serialization/deserialization errors. There is currently no test which ensures that these definitions are in sync, and (surprise!) they were NOT in sync until very recently - luckily so far just in case of (mostly) unused fields that do not matter that much, or with regard to the human readable descriptions. The bulk of the long slew of PRs referenced above this comment where I made sure they are back in sync.

Now, my plan going forward is not to introduce tests that try to detect whether these are in sync by round-tripping data back and forth, because that is going to be tedious and error-prone, and be just as susceptible to bit-rot (i.e. we'd need to remember to test new fields as we add them) as the current situation.

My plan instead is to make the structs in the apis package into the single source of truth, i.e. have the CRDs YAML generated from them using controller-gen, and then have the resulting YAML embedded into the kudoinit package with go-bindata, getting rid of that duplicate definition.

In order to optimize for review speed and quality I'm doing this in small chunks each of which is obvious and quick to review. Because if I did it all in one go, there would be a 300-500 line YAML diff, which would be impossible to make sure is correct.

So I'm kind of trying to avoid the need for round-trip testing in the first place (will update the summary in a moment).
We'll effectively replace this with trusting controller-gen, but I think it's a good trade-off.

@porridge porridge changed the title Write Test for Round Trip Testing of CRDs Make sure CRDs are in sync with their counterpart Go structs Dec 19, 2019
@alenkacz
Copy link
Contributor

sounds good 👍

porridge added a commit that referenced this issue Dec 19, 2019
Result of discussion in #1198.

It was never used (verified by eyeballing git log -p all the way
down), we don't seem to know what it's for, and it has no counterpart in
the structs.

This is a baby step towards #862.
porridge added a commit that referenced this issue Dec 20, 2019
This will help reduce the size of the diff when we move to
auto-generating these files with controller-gen.

This a baby-step towards #862
porridge added a commit that referenced this issue Jan 8, 2020
My guess is that they were meant for https://github.com/go-validator/validator
but this library does not seem to be used (any more?).

We do not seem to be using these tags for anything, they are not enforced, and
the current codebase violates them.

I did try to make the CRD validation markers match the tags in the process of
fixing #862, but there is plenty of test code which violates those rules, so it
would be a bigger task at this point.

Since these tags contradict the +optional markers on directly preceding lines,
it is best we reduce confusion by removing them.
ANeumann82 pushed a commit that referenced this issue Feb 13, 2020
It seems to me like using the full-blown type with spec, status and all had been a mistake.
This field seems unused in the code, so maybe that is why this was missed.

This is a baby step towards #862.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82 pushed a commit that referenced this issue Feb 13, 2020
Result of discussion in #1198.

It was never used (verified by eyeballing git log -p all the way
down), we don't seem to know what it's for, and it has no counterpart in
the structs.

This is a baby step towards #862.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82 pushed a commit that referenced this issue Feb 13, 2020
This will help reduce the size of the diff when we move to
auto-generating these files with controller-gen.

This a baby-step towards #862

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82 pushed a commit that referenced this issue Feb 13, 2020
My guess is that they were meant for https://github.com/go-validator/validator
but this library does not seem to be used (any more?).

We do not seem to be using these tags for anything, they are not enforced, and
the current codebase violates them.

I did try to make the CRD validation markers match the tags in the process of
fixing #862, but there is plenty of test code which violates those rules, so it
would be a bigger task at this point.

Since these tags contradict the +optional markers on directly preceding lines,
it is best we reduce confusion by removing them.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants