Use gengo/v2 in kube-openapi #458
Conversation
k8s-ci-robot
added
cncf-cla: yes
| "the name of the file to be generated") | ||
| fs.StringVar(&args.GoHeaderFile, "go-header-file", "", | ||
| "the path to a file containing boilerplate header text; the string \"YEAR\" will be replaced with the current 4-digit year") | ||
| fs.StringVarP(&args.ReportFilename, "report-filename", "r", "-", |
There was a problem hiding this comment.
nit: use the c.ReportFilename current field value (defaulted in New()) rather than stomping a hard-coded string - here?
pkg/generators/config.go
Outdated
| GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { | ||
| return []generator.Target{ | ||
| &generator.SimpleTarget{ | ||
| PkgName: filepath.Base(args.OutputDir), |
There was a problem hiding this comment.
is filepath.Base(args.OutputDir) actually correct for finding the package name, or is it best-effort / usually-correct? if the latter, comment here to note that limitation and possibly for a follow-up improvement
would path.Base(args.OutputPackage) be more likely to be correct (though still possibly best-effort) - path instead of filepath, customArgs.OutputPackage instead of arguments.OutputBase?
There was a problem hiding this comment.
Correct on Pkg vs Dir.
filepath touches a few more places.
|
hmm, integration test CI looks unhappy |
|
Yeah, I am looking at it. TL;DR: it seems to run openapi-gen and then re-run it witrh --verify. I must be misunderstanding it. |
|
We have a separate go.mod file for the integration test directory. I'm assuming the go.mod there needs to be updated as well |
This does not fix all the logic. That will be done subsequently.
TO REPEAT:
git grep -l 'k8s.io/gengo/[^v][^2]' \
| while read F; do \
sed -i 's|k8s.io/gengo/\([^v]\)|k8s.io/gengo/v2/\1|' $F
done
Also get rid of targetPackage - it's not a package, it's a directory. We can't pretend an arbitrary output dir is a Go package or that the directory path has anything to do with the package name. Go is particular in its definition - a package has at least one .go file in it. If the output dir doesn't (yet), then it's not a resolvable package - even if it SEEMS like it should be. Fortunately, since the directory we emit to is never one of the input directories, we don't need to try to elide the "local" package name from type references. Also, we don't actually make any such type references. If that were to change, we could easily adapt - scan the inputs' SourcePaths for the output dir and, IFF found, pass it down. But the odds of needing that are ~0 at this point.
If it does fail, it is catastophic. Don't bother trying to handle it.
...and rename to --output-dir
The changes appear large but are mostly mechanical. Understand one and you pretty much understand them all.
|
Test should pass now. The integration test at HEAD is B-R-O-K-E-N. Running it man ually leaves uncommitted changes, but the test passes. The test itself runs openapi-gen (which generates a delta) then runs a verify against the fresh delta, which passes. The test could literally never fail (unless it exits non-zero). I don't know why the delta exists. It seems to be a regression in formatting? See d1213fd If the delta is expected, it's fixed here. If not, someone show me why? This PR now changes the test to generate code into the temp dir and diff, just like it does for the reports and other files. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
k8s-ci-robot
added
the
do-not-merge/hold
|
I can't really fix k/k until this merges, but almost all of these commits came straight out of my k/k tree /remove-hold |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/hold cancel |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski, liggitt, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This is needed to support Go workspaces in k/k.
This is done as several small commits so each step can be seen. It has the unfortunate effect of not compiling at each step.
@liggitt @alexzielenski @jpbetz