Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

align on controller-gen 0.4.0 #370

Merged

Conversation

jaypipes
Copy link
Collaborator

@jaypipes jaypipes commented Oct 5, 2020

We need to ensure that all ACK developers are using the same
controller-gen version otherwise we get into situations where:

  1. go.mod/go.sum change unnecessarily
  2. The annotations for things like CRDs will be different for generated
    YAML manifests

This patch makes v0.4.0 of the controller-tools repo and controller-gen
binary our target version. It changes the ensure_controller_gen Bash
function to ensure that controller-gen at that version is installed and
refuses to proceed with building controllers if there is a different
version.

jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ controller-gen --version
Version: v0.3.1-0.20200716001835-4a903ddb7005
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ make build-controller SERVICE=dynamodb
make[1]: Entering directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
go build -tags codegen -ldflags "-X main.version="v0.0.0" -X main.buildHash=598a3e29bb514d98660d04a088641922ccc020c9 -X main.buildDate=2020-10-05T16:52:56Z" -o bin/ack-generate cmd/ack-generate/main.go
./scripts/build-controller.sh dynamodb
FAIL: Existing version of controller-gen Version: v0.3.1-0.20200716001835-4a903ddb7005, required version is v0.4.0.
FAIL: Please uninstall controller-gen and re-run this script, which will install the required version.
make[1]: *** [Makefile:49: build-controller] Error 1
make[1]: Leaving directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ sudo rm -f `which controller-gen`
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ make build-controller SERVICE=dynamodb
make[1]: Entering directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
go build -tags codegen -ldflags "-X main.version="v0.0.0" -X main.buildHash=598a3e29bb514d98660d04a088641922ccc020c9 -X main.buildDate=2020-10-05T16:53:07Z" -o bin/ack-generate cmd/ack-generate/main.go
./scripts/build-controller.sh dynamodb
go: creating new go.mod: module tmp
go: found sigs.k8s.io/controller-tools/cmd/controller-gen in sigs.k8s.io/controller-tools v0.4.0
****************************************************************************
WARNING: You may need to reload your Bash shell and path. If you see an
         error like this following:

Error: couldn't find github.com/aws/aws-sdk-go in the go.mod require block

simply reload your Bash shell with `exec bash` and then re-run whichever
command you were running.
****************************************************************************
Building Kubernetes API objects for dynamodb
Error: couldn't find github.com/aws/aws-sdk-go in the go.mod require block
make[1]: *** [Makefile:49: build-controller] Error 2
make[1]: Leaving directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ exec bash
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ make build-controller SERVICE=dynamodb
make[1]: Entering directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
go build -tags codegen -ldflags "-X main.version="v0.0.0" -X main.buildHash=598a3e29bb514d98660d04a088641922ccc020c9 -X main.buildDate=2020-10-05T16:53:21Z" -o bin/ack-generate cmd/ack-generate/main.go
./scripts/build-controller.sh dynamodb
Building Kubernetes API objects for dynamodb
Generating deepcopy code for dynamodb
Generating custom resource definitions for dynamodb
Building service controller for dynamodb
Generating RBAC manifests for dynamodb
Running gofmt against generated code for dynamodb
make[1]: Leaving directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'

Issue #349

Related: kubernetes-sigs/controller-tools#500

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jaypipes
Copy link
Collaborator Author

jaypipes commented Oct 5, 2020

Note to reviewers: I will regenerate all service controllers to align with the 0.4.0 controller-tools version in a separate, single commit.

@jaypipes jaypipes requested a review from a-hilaly October 5, 2020 17:57
echo "Existing version of controller-gen "`controller-gen --version`", minimum required is $minimum_req_version"
exit 1
if ! is_installed controller-gen; then
__install_path=/usr/local/bin/controller-gen
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users might have different Go install directories. Maybe let's use a more generic GOBIN path ? something like this should work

__install_path=$(go env GOBIN)/controller-gen

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other things (like kind) are being installed into /usr/local/bin but OK :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I personally don't have the GOBIN env var set... it's not set by default on Linux IIRC. You need to set it manually, even though $GOPATH/bin is the default for it:

jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ go env GOBIN

jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ go env GOPATH
/home/jaypipes/go
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ ll $GOPATH
total 20
drwxr-xr-x  5 jaypipes jaypipes 4096 Aug 22  2019 ./
drwxr-xr-x 31 jaypipes jaypipes 4096 Oct  6 07:53 ../
drwxr-xr-x  2 jaypipes jaypipes 4096 Oct  6 07:53 bin/
drwxr-xr-x  5 jaypipes jaypipes 4096 Apr  5  2020 pkg/
drwxr-xr-x  8 jaypipes jaypipes 4096 Aug 25  2019 src/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other things (like kind) are being installed into /usr/local/bin but OK :)

Ohh right! Forgot about kind case

Also, I personally don't have the GOBIN env var set... it's not set by default on Linux IIRC. You need to set it manually, even though $GOPATH/bin is the default for it:

I always thought that go env doesn't use the OS env vars, i just found out that i have a GOBIN set in my .bash_profile... pre go1.11 habits :)

Comment on lines +20 to +30
go get -d "sigs.k8s.io/controller-tools/cmd/controller-gen@${CONTROLLER_TOOLS_VERSION}"
go build -o "$__work_dir/controller-gen" sigs.k8s.io/controller-tools/cmd/controller-gen
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL :)

exit 1
if ! is_installed controller-gen; then
__install_path=/usr/local/bin/controller-gen
__work_dir=$(mktemp --tmpdir -d controller-gen-XXX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this script is working on Debian-based Linux ditributions at least, however mktemp on Darwin works differently 😕 . Basically on darwin, mktemp doesn't accept a --tmpdir flag along with small other differences.

Instead of dealing with each case separately, we can just use the common -d flag, and hard code (or use a $TMPDIR environment variable) the temp directory.
Using mktemp -d /tmp/controller-gen-XXX seems to be the right/fairest solutions for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k, will update.

We need to ensure that all ACK developers are using the same
controller-gen version otherwise we get into situations where:

1) go.mod/go.sum change unnecessarily
2) The annotations for things like CRDs will be different for generated
   YAML manifests

This patch makes v0.4.0 of the controller-tools repo and controller-gen
binary our target version. It changes the `ensure_controller_gen` Bash
function to ensure that controller-gen at that version is installed and
refuses to proceed with building controllers if there is a different
version.

```
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ controller-gen --version
Version: v0.3.1-0.20200716001835-4a903ddb7005
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ make build-controller SERVICE=dynamodb
make[1]: Entering directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
go build -tags codegen -ldflags "-X main.version="v0.0.0" -X main.buildHash=598a3e29bb514d98660d04a088641922ccc020c9 -X main.buildDate=2020-10-05T16:52:56Z" -o bin/ack-generate cmd/ack-generate/main.go
./scripts/build-controller.sh dynamodb
FAIL: Existing version of controller-gen Version: v0.3.1-0.20200716001835-4a903ddb7005, required version is v0.4.0.
FAIL: Please uninstall controller-gen and re-run this script, which will install the required version.
make[1]: *** [Makefile:49: build-controller] Error 1
make[1]: Leaving directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ sudo rm -f `which controller-gen`
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ make build-controller SERVICE=dynamodb
make[1]: Entering directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
go build -tags codegen -ldflags "-X main.version="v0.0.0" -X main.buildHash=598a3e29bb514d98660d04a088641922ccc020c9 -X main.buildDate=2020-10-05T16:53:07Z" -o bin/ack-generate cmd/ack-generate/main.go
./scripts/build-controller.sh dynamodb
go: creating new go.mod: module tmp
go: found sigs.k8s.io/controller-tools/cmd/controller-gen in sigs.k8s.io/controller-tools v0.4.0
****************************************************************************
WARNING: You may need to reload your Bash shell and path. If you see an
         error like this following:

Error: couldn't find github.com/aws/aws-sdk-go in the go.mod require block

simply reload your Bash shell with `exec bash` and then re-run whichever
command you were running.
****************************************************************************
Building Kubernetes API objects for dynamodb
Error: couldn't find github.com/aws/aws-sdk-go in the go.mod require block
make[1]: *** [Makefile:49: build-controller] Error 2
make[1]: Leaving directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ exec bash
jaypipes@thelio:~/go/src/github.com/aws/aws-controllers-k8s$ make build-controller SERVICE=dynamodb
make[1]: Entering directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
go build -tags codegen -ldflags "-X main.version="v0.0.0" -X main.buildHash=598a3e29bb514d98660d04a088641922ccc020c9 -X main.buildDate=2020-10-05T16:53:21Z" -o bin/ack-generate cmd/ack-generate/main.go
./scripts/build-controller.sh dynamodb
Building Kubernetes API objects for dynamodb
Generating deepcopy code for dynamodb
Generating custom resource definitions for dynamodb
Building service controller for dynamodb
Generating RBAC manifests for dynamodb
Running gofmt against generated code for dynamodb
make[1]: Leaving directory '/home/jaypipes/go/src/github.com/aws/aws-controllers-k8s'
```

Issue aws-controllers-k8s#349

Related: kubernetes-sigs/controller-tools#500
@jaypipes jaypipes force-pushed the controller-gen-standard branch from d360c5a to ae3179a Compare October 6, 2020 12:00
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm 👍

Works perfectly on my machine

@jaypipes jaypipes merged commit b7c5163 into aws-controllers-k8s:main Oct 6, 2020
@jaypipes jaypipes deleted the controller-gen-standard branch October 6, 2020 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants