Closed KnVerey closed 12 months ago
@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.
/assign
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.
prow-presubmit-check
? 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.
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, CMIIWpSrc
with the ones in pGen
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
.
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?
One more thing, what kind of handling is expected upon finding diff?
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
Thanks so much @antoooks!
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 changesgo work sync
as well, now that we're using workspace mode.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.