Fix config/crd/bases vs helm/crds list-marker mismatch (gofmt ordering) - #737
Fix config/crd/bases vs helm/crds list-marker mismatch (gofmt ordering)#737HansG89 wants to merge 1 commit into
Conversation
controller-gen crd runs twice per service across build-controller.sh (config/crd/bases) and build-controller-release.sh (helm/crds), reading from the same generated apis/ Go source tree. gofmt's doc-comment reformatter rewrites Markdown "*" list markers to "-" (Go 1.19+), but previously only ran once, at the very end of build-controller.sh, after the config/crd/bases controller-gen call but before the build-controller-release.sh one. So for any field whose doc comment has a bullet list, config/crd/bases and helm/crds permanently disagree on marker style for identical text -- regenerating always flips one file's CRD YAML relative to the other's. Move the gofmt call to run immediately after ack-generate produces the apis/ source, before either controller-gen crd invocation, so both capture doc comments on the same side of gofmt's rewrite.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: HansG89 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ack-prow
Bot
added
the
needs-ok-to-test
|
Hi @HansG89. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Regenerates the controller using aws-controllers-k8s/code-generator#737, which moves gofmt's doc-comment normalization to run before either controller-gen crd invocation, instead of only after the first one. Without this fix, config/crd/bases/*.yaml and helm/crds/*.yaml disagree on Markdown list-marker style (* vs -) for the same enum description text (e.g. Job.spec.jobMode), which is what was failing glue-verify-code-gen on this PR. Both CRD YAML targets now agree.
|
@a-hilaly @knottnt pinging since this is currently blocking glue-controller#16's CI ( Would appreciate a look when you get a chance, since it's the last thing standing between glue#16 and a mergeable state. Thank you! |
|
@HansG89 I don't think this is the root cause of the verify-code-gen failures in aws-controllers-k8s/glue-controller#16. That CI test checks whether or not the CI generated files match what's in the PR and not whether Helm vs config/crd/bases are identical. |
You're right. PR16's failure was code-generator#733, not the list-marker issue this PR fixes. Merging main into PR16 picked up #733 and resynced types.go. Will update this PR's description to drop the glue#16 reference The bullet-marker divergence is still a real, separate bug. Should we keep this one or close it? |
3 participants
Problem
For any generated field whose Go doc comment contains a Markdown bullet list (e.g. an enum's valid-values doc),
config/crd/bases/*.yamlandhelm/crds/*.yamlpermanently disagree on the list-marker style (*vs-) for identical description text.Example live on
aws-controllers-k8s/glue-controllermain today (Job.spec.jobModedoc):Root cause
config/crd/basesandhelm/crdsare each produced by their owncontroller-gen crdcall against the same generatedapis/<version>source, but on opposite sides of agofmtpass:scripts/build-controller.shrunscontroller-gen crdforconfig/crd/basesbefore its end-of-scriptgofmt -w, whilescripts/build-controller-release.sh(run right after, samemake build-controller) runs its owncontroller-gen crdforhelm/crdsagainst the now-gofmt'd source.gofmtrewrites*-style Markdown list markers in doc comments to-, so the two targets capture the same comment before/after that rewrite and disagree forever.Fix
Move the doc-comment-normalizing
gofmt -wto run right afterack-generate apis, before eithercontroller-gen crdcall. No-op on already-formatted source.Verification
make build-controller SERVICE=gluetwice back-to-back:config/crd/basesandhelm/crdsnow byte-identical forjobModeand the rest of the schema; second run idempotent.cc @michaelhtm