Some CI tasks should include verification that no diff was produced

[KnVerey]

• make test-go-mod is run part of presubmit (though we're considering replacing it), but it is not very effective on its own as a CI check, because it passes even if the command makes uncommitted changes
  • The CI check should probably do go work sync as well, now that we're using workspace mode.
• I don't think make generate-kustomize-builtin-plugins is currently run at all. It should be run, with an assertion that it doesn't generate a diff. This implies we're relying on manual reviews of the generated files + e2e tests (typically Krusty tests) having enough coverage to detect unintended drift.

[k8s-ci-robot]

@KnVerey: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions 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/test-infra repository.

[antoooks]

/assign

[antoooks]

Hi @KnVerey,

I am quite new to the codebase and would like to clarify on the requirements and familiarise myself with the repo.

make test-go-mod is run part of presubmit (though we're considering replacing it), but it is not very effective on its own as a CI check, because it passes even if the command makes uncommitted changes

• The CI check should probably do go work sync as well, now that we're using workspace mode.

• Does "CI check" you mentioned refers to Github action step-level or the Makefile-level check namely prow-presubmit-check ?
• In case of Make-file level check should I make the go work sync as one of targets under presubmit-check?

I don't think make generate-kustomize-builtin-plugins is currently run at all. It should be run, with an assertion that it doesn't generate a diff. This implies we're relying on manual reviews of the generated files + e2e tests (typically Krusty tests) having enough coverage to detect unintended drift.

• This make generate-kustomize-builtin-plugins is expected to generate files explicitly specified in _builtplugins under pGen directory if I deleted those files, which now is not able to run properly, CMIIW
• I take it that this requires the test to check diffs between current branch with the master branch after the files are successfully generated? Could you elaborate more?
• Or... the second assumption is to compare diff between files in pSrc with the ones in pGen

[natasha41575]

Does "CI check" you mentioned refers to Github action step-level or the Makefile-level check namely prow-presubmit-check ?

I believe that prow-presubmit-check is run by Github actions so it should be sufficient to add it there.

To answer the rest of your questions, I think generate-kustomize-builtin-plugins is supposed to generate the code under api/internal/builtins. (If that one doesn't, then there should be one that does.) The CI check should run that target, make test-go-mod and go work sync, and then verify that it didn't produce any diff from before running those and after running those. So not between current branch and master, but between current branch before running generate-kustomize-builtin-plugins, make test-go-mod and go work sync, and current branch after running those.

You can probably define a new make target like make no-diff or something like that, and call it from under prow-presubmit-check.

[antoooks]

Super! Thanks @natasha41575 for the insights

I think of having the same strategy for make no-diff, so we basically compares HEAD with current working tree, in this case we can approach this using 2 ways:

Opt 1: All done within Makefile, calling a simple git command, but the check will be very simple and it will not cover any intelligent logic, plus it will need the CI environment to have git
Opt 2: Create a separate module like github.com/joelanford/go-apidiff@v0.6.0, it decouples better, and can be customised to be more intelligent, but it will introduce maintenance overhead compared to option 1, and requires the module to be stable all the time

Alternatively, if we aim to check the file integrity, we can use built-in commands like md5sum or cmp command instead, since it is simpler and serves the purpose

Wdyt?

[antoooks]

One more thing, what kind of handling is expected upon finding diff?

[antoooks]

As per last standup meeting, we go with option 2 (to have a different module to check diff, utilising gorepomod) and we throwing error is expected upon finding diff.

cc @natasha41575

[antoooks]

Resolved by #5303

[natasha41575]

Thanks so much @antoooks!