introduce crd-schema-checker#1055
Conversation
dgn
force-pushed
the
crd-compatibility-check
branch
from
a002f63 to
a5334e7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1055 +/- ##
==========================================
- Coverage 81.20% 80.76% -0.45%
==========================================
Files 46 46
Lines 2469 2469
==========================================
- Hits 2005 1994 -11
- Misses 341 348 +7
- Partials 123 127 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dgn
force-pushed
the
crd-compatibility-check
branch
9 times, most recently
from
26f855e to
e4babbe
Compare
dgn
force-pushed
the
crd-compatibility-check
branch
3 times, most recently
from
3d32594 to
085fd03
Compare
|
I updated this, it was pretty broken before. Now it actually outputs all the errors, warnings and info messages and has a proper global error count, and a separate one for errors in stable APIs (as only those will fail the check) |
dgn
force-pushed
the
crd-compatibility-check
branch
from
085fd03 to
0063f73
Compare
| @test -s $(LOCALBIN)/misspell || GOBIN=$(LOCALBIN) go install github.com/client9/misspell/cmd/misspell@$(MISSPELL_VERSION) | ||
|
|
||
| .PHONY: crd-schema-checker | ||
| crd-schema-checker: $(CRD_SCHEMA_CHECKER) ## Download crd-schema-checker to bin directory. |
There was a problem hiding this comment.
Why even have the phony target?
It seems to me lint-crds can just depend on $(CRD_SCHEMA_CHECKER) directly
There was a problem hiding this comment.
technically yes, but that's how we're doing it for all other tools as well
| echo " - $line" | ||
| done <<< "$output" | ||
| fi | ||
| echo "--> ${errors} errors, ${warnings} warnings, ${infos} infos" |
There was a problem hiding this comment.
Why is it pertinent to count the errors/warn/info here?
Wouldn't it make more sense to push a PR to print that in the original command anyway?
There was a problem hiding this comment.
yeah we need to count, if only to separate them by stable vs. non-stable APIs- we can allow breaking changes in v1alpha1 CRDs, but never in v1 CRDs, for example
| echo "=== Results ===" | ||
| echo "Checked $CHECKED_CRDS CRDs: $ERRORS errors ($STABLE_ERRORS errors in stable APIs), $WARNINGS warnings, $INFOS infos" | ||
|
|
||
| if [[ $STABLE_ERRORS -gt 0 ]]; then |
There was a problem hiding this comment.
Generally to me parsing the output seems like the wrong approach.
We have the exit code of the checks, we should rely on those instead of trying to read and interpret the output.
There was a problem hiding this comment.
again, we need to parse to get detailed results - warnings we can always ignore, for pre-release CRDs we can even ignore errors. yes, technically we could do all that based on just exit codes but we wouldn't be able to produce detailed counts
|
/retest |
|
This seems to be a legitimate error: Did we forget to add 1.26.1 on the 1.27 branch? |
dgn
force-pushed
the
crd-compatibility-check
branch
from
0063f73 to
fc2bcf3
Compare
We have previously released a version on the 1.26 stream that supported 1.26.1. In order to not break API guarantees, that enum value of 1.26.1 has to be supported in later releases as well, so I'm adding it here. This problem was found with the crd-schema-checker introduced in istio-ecosystem#1055. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
dgn
force-pushed
the
crd-compatibility-check
branch
from
fc2bcf3 to
8fb3f20
Compare
This was indeed the case. First legit problem the checker found 👍 Fix is in #1228, until it merges the lint job will fail on this PR. |
We have previously released a version on the 1.26 stream that supported 1.26.1. In order to not break API guarantees, that enum value of 1.26.1 has to be supported in later releases as well, so I'm adding it here. This problem was found with the crd-schema-checker introduced in #1055. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
|
/retest |
|
LGTM, not approving to wait for other comments to be resolved. |
dgn
force-pushed
the
crd-compatibility-check
branch
from
8fb3f20 to
b4e4ede
Compare
|
currently if not on a release branch, the linter will compare the last two release branches. We should change that to compare the current HEAD against the latest release branch |
dgn
force-pushed
the
crd-compatibility-check
branch
from
b4e4ede to
2be5426
Compare
|
lint job will fail until #1301 is merged |
dgn
force-pushed
the
crd-compatibility-check
branch
from
2be5426 to
ea7193b
Compare
|
/retest |
nrfox
left a comment
nrfox
left a comment
There was a problem hiding this comment.
Changes LGTM.
Does this still need to be cherry-picked to 1.27? If so, should it also be cherry-picked to 1.28? I think either way the PR needs to rebased so it can run against current main.
dgn
force-pushed
the
crd-compatibility-check
branch
from
ea7193b to
55373ce
Compare
dgn
force-pushed
the
crd-compatibility-check
branch
from
55373ce to
90f3ae4
Compare
|
Rebased this once more. PTAL |
This is a script that uses openshift/crd-schema-checker to verify that we're not breaking any CRD API guarantees across releases. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
dgn
force-pushed
the
crd-compatibility-check
branch
from
90f3ae4 to
b5c7183
Compare
|
/retest |
|
/retest |
6 participants
This is a script that uses openshift/crd-schema-checker to verify that we're not breaking any CRD API guarantees across releases.
We can't merge this until we fix the errors, but it's pretty neat.