✨ Make apigen a standalone go module #2669
Conversation
ncdc
requested review from
stevekuznetsov,
clubanderson,
MikeSpreitzer and
sttts
and removed request for
s-urbaniak and
csams
|
/lgtm |
hack/verify-go-modules.sh
Outdated
| mapfile -t DIRS < <(find "${REPO_ROOT}" -name go.mod -print0 | xargs -0 dirname) | ||
|
|
||
| for dir in "${DIRS[@]}"; do | ||
| pushd "$dir" &> /dev/null |
There was a problem hiding this comment.
Using ( cd $dir; ... ) is safer than pushd $dir; ...; popd
There was a problem hiding this comment.
I do not know how to test that it solves the problem. I tried and failed as follows.
(base) mspreitz@mjs12 test5 % go install github.com/ncdc/kcp/cmd/apigen@bd5342130765ecec91053b5690e9e246c41372df
go: downloading github.com/ncdc/kcp/cmd/apigen v0.0.0-20230124185555-bd5342130765
go: downloading github.com/ncdc/kcp v0.0.0-20230124185555-bd5342130765
go: github.com/ncdc/kcp/cmd/apigen@bd5342130765ecec91053b5690e9e246c41372df: github.com/ncdc/kcp/cmd/[email protected]: parsing go.mod:
module declares its path as: github.com/kcp-dev/kcp/cmd/apigen
but was required as: github.com/ncdc/kcp/cmd/apigen
|
I see that this avoids the "replace" for apis by requiring a specific version rather than replacing with the version in the same commit. This requires updating the "require" line every time the apis change. Just checking that this consequence is understood and approved by others. |
|
|
||
| REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) | ||
|
|
||
| mapfile -t DIRS < <(find "${REPO_ROOT}" -name go.mod -print0 | xargs -0 dirname) |
There was a problem hiding this comment.
dirname terminates its output with a newline, right? So despite all the effort, this still fails when a directory name includes a newline, right? There are simpler approaches that accomplish as much. OTOH, if you want to succeed when the directory name contains a newline, perhaps find ... -exec process.sh \{\} \;.
There was a problem hiding this comment.
I am following the shellcheck suggestions
There was a problem hiding this comment.
The following makes shellcheck happy and works with newlines in the directory name. It starts with a listing showing all the paths used in this test.
bash-5.1$ find . -name go.mod
./ta
ste/go.mod
./go.mod
./docs/go.mod
./te st/go.mod
./pkg/apis/go.mod
bash-5.1$ cat > process.sh
#!/usr/bin/env bash
echo "Working on (${1%/go.mod})"
bash-5.1$ chmod a+x process.sh
bash-5.1$ find . -name go.mod -exec "$PWD/process.sh" \{\} \;
Working on (./ta
ste)
Working on (.)
Working on (./docs)
Working on (./te st)
Working on (./pkg/apis)
This will only work after it merges.
We only have to update if/when the code in apigen's main.go needs to react to changes to the apis such that the generation logic needs to change. For typical api modifications, we shouldn't need to change anything. |
Signed-off-by: Andy Goldstein <[email protected]>
Signed-off-by: Andy Goldstein <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc 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 |
openshift-ci
bot
added
the
approved
|
Thanks! |
|
Still having trouble testing. |
|
It looks like you're still trying to use my $ cd /tmp
$ mkdir foo
$ cd foo
$ go install github.com/kcp-dev/kcp/cmd/apigen@main
go: downloading github.com/kcp-dev/kcp/cmd/apigen v0.0.0-20230124223207-cb8dd3c46470
go: downloading github.com/kcp-dev/kcp v0.10.1-0.20230124223207-cb8dd3c46470 |
|
D'oh! Yes, it works when I do it right. |
Summary
Make apigen a standalone go module so it can be installed by itself.
Related issue(s)
Fixes #2668