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.

Sign up for GitHub

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

Jump to bottom

Add build info for CAPX #1313

Merged
merged 8 commits into from
Sep 29, 2022
Merged

Add build info for CAPX #1313

merged 8 commits into from
Sep 29, 2022

Conversation

thunderboltsid
Copy link
Contributor

CAPX is Nutanix's Cluster API provider.

@eks-distro-bot
Copy link
Collaborator

Hi @thunderboltsid. Thanks for your PR.

I'm waiting for a aws member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@eks-distro-bot eks-distro-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 19, 2022
@vignesh-goutham
Copy link
Member

Can you please add a presubmits job for CAPX here.

/ok-to-test

@deepakm-ntnx
Copy link
Contributor

@thunderboltsid could you please list the steps in description if any make commands were used to generate any of these files or in general how were these filed created even if they were referenced from some other project for information

@thunderboltsid thunderboltsid force-pushed the nutanix branch 3 times, most recently from ea78bc3 to 17b01f6 Compare September 20, 2022 13:42
@thunderboltsid
Copy link
Contributor Author

thunderboltsid commented Sep 20, 2022 •
edited
Loading

@thunderboltsid could you please list the steps in description if any make commands were used to generate any of these files or in general how were these filed created even if they were referenced from some other project for information

Please see the README.md checked-in along with the changes

@thunderboltsid
Copy link
Contributor Author

thunderboltsid commented Sep 20, 2022 •
edited
Loading

here

Sure.

@thunderboltsid thunderboltsid force-pushed the nutanix branch 3 times, most recently from 7d160c5 to 00e5e86 Compare September 20, 2022 14:59
@thunderboltsid
Copy link
Contributor Author

/assign abhinavmpandey08

@thunderboltsid thunderboltsid mentioned this pull request Sep 21, 2022
Merged
@abhinavmpandey08
Copy link
Member

/hold
Need to do some infra changes on our side for this to work. I will un-hold the PR once that's done

@thunderboltsid
Copy link
Contributor Author

/hold
Need to do some infra changes on our side for this to work. I will un-hold the PR once that's done

out of curiosity, can you elaborate on the changes needed?

@abhinavmpandey08
Copy link
Member

/hold
Need to do some infra changes on our side for this to work. I will un-hold the PR once that's done

out of curiosity, can you elaborate on the changes needed?

It's mainly setting CI pipelines which will handle CAPX builds and creating ECR repos where the CAPX images will be published

@abhinavmpandey08
Copy link
Member

/unhold

thunderboltsid
thunderboltsid commented Sep 27, 2022
View reviewed changes
build/lib/install_go_versions.sh
Comment on lines +57 to +58
build::common::use_go_version "1.19"
GOBIN=${GOPATH}/go1.19/bin go install github.com/google/[email protected]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this the make commands default togo1.16

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to take care of it as mentioned here google/go-licenses#128

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the latest run, switching to 1.0.0 doesn't seem to have an effect on the CI run. I don't see this locally on my machine but I have the 1.0.0 version of the binary. Switching the local binary to 1.2.1 I am able to repro the issue.

@thunderboltsid thunderboltsid force-pushed the nutanix branch 2 times, most recently from 9e68c29 to c4c6e4c Compare September 27, 2022 21:07
@thunderboltsid
Copy link
Contributor Author

/retest-required

@thunderboltsid
Copy link
Contributor Author

thunderboltsid commented Sep 27, 2022 •
edited
Loading

@thunderboltsid: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
cluster-api-provider-nutanix-tooling-presubmit c4c6e4c link true /test cluster-api-provider-nutanix-tooling-presubmit
Full PR test history. Your PR dashboard.

Seems like it ends up using 1.16 when passing 1.19; this wasn't happening with 1.17. Any ideas @abhinavmpandey08 @jaxesn ?

/home/prow/go/src/github.com/aws/eks-anywhere-build-tooling/build/lib/gather_licenses.sh cluster-api-provider-nutanix /home/prow/go/src/github.com/aws/eks-anywhere-build-tooling/projects/nutanix-cloud-native/cluster-api-provider-nutanix/_output/ "." . "1.19"
321
Adding /go/go1.16/bin to PATH
322
vendor/sigs.k8s.io/json/json.go:24:2: //go:build comment without // +build comment
323
make: *** [_output/attribution/go-license.csv] Error 1

@thunderboltsid
Add build info for CAPX
8f1ad93 
CAPX is Nutanix's Cluster API provider.
@thunderboltsid
Update common.sh
f360b92 
Use go1.19 for gather licenses if present.
@thunderboltsid
Change go-licenses version for 1.19
c98fad5
@thunderboltsid
Copy link
Contributor Author

Seems like it ends up using 1.16 when passing 1.19; this wasn't happening with 1.17. Any ideas @abhinavmpandey08 @jaxesn ?

/home/prow/go/src/github.com/aws/eks-anywhere-build-tooling/build/lib/gather_licenses.sh cluster-api-provider-nutanix /home/prow/go/src/github.com/aws/eks-anywhere-build-tooling/projects/nutanix-cloud-native/cluster-api-provider-nutanix/_output/ "." . "1.19"
321
Adding /go/go1.16/bin to PATH
322
vendor/sigs.k8s.io/json/json.go:24:2: //go:build comment without // +build comment
323
make: *** [_output/attribution/go-license.csv] Error 1

This is fixed now by ensuring the right go version 1.19 has it's own case in the build scripts.

@thunderboltsid
Copy link
Contributor Author

thunderboltsid commented Sep 28, 2022 •
edited
Loading

The current issue we have seems to stem from the go-licenses version. It looks like the go-licenses version set in the 1.19 case is not the one being used.

/home/prow/go/src/github.com/aws/eks-anywhere-build-tooling/build/lib/gather_licenses.sh cluster-api-provider-nutanix /home/prow/go/src/github.com/aws/eks-anywhere-build-tooling/projects/nutanix-cloud-native/cluster-api-provider-nutanix/_output/ "." . "1.19"
Adding /usr/bin to PATH
E0927 23:17:10.331666    9287 library.go:110] Package flag does not have module info. Non go modules projects are no longer supported. For feedback, refer to https://github.com/google/go-licenses/issues/128.

I'd have expected the path added to be /go/go1.19/bin not /usr/bin

@thunderboltsid
Copy link
Contributor Author

/retest-required

@thunderboltsid
Copy link
Contributor Author

thunderboltsid commented Sep 28, 2022 •
edited
Loading

I'd have expected the path added to be /go/go1.19/bin not /usr/bin

Looking inside the builder base image

➜  aws-eks-anywhere-build-tooling git:(nutanix) docker run -it public.ecr.aws/eks-distro-build-tooling/builder-base:d92cba76cf18066bf37ee29dedbd3ced5b1aae86.2 bash
bash-4.3# ls /go
bin  go1.13  go1.14  go1.15  go1.16  go1.17  go1.18  pkg  src

The latest tag has the same issue.

➜  aws-eks-anywhere-build-tooling git:(nutanix) docker run -it public.ecr.aws/eks-distro-build-tooling/builder-base:latest bash
bash-4.3# ls /go
bin  go1.13  go1.14  go1.15  go1.16  go1.17  go1.18  pkg  src

@abhinavmpandey08 @jaxesn I think we might need a new builder-base image with go1.19. I had hoped that the setupgo directive in install_go_versions.sh would've taken care of that.

@abhay-krishna
Copy link
Member

/lgtm
/approve

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhay-krishna

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@abhinavmpandey08
Copy link
Member

/woof

@eks-distro-bot
Copy link
Collaborator

@abhinavmpandey08: dog image

In response to this:

/woof

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.

@eks-distro-bot eks-distro-bot merged commit bba5ae1 into aws:main Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepakm-ntnx deepakm-ntnx deepakm-ntnx left review comments

@abhay-krishna abhay-krishna abhay-krishna left review comments

@jaxesn jaxesn jaxesn left review comments

@abhinavmpandey08 abhinavmpandey08 abhinavmpandey08 approved these changes

Assignees

@abhinavmpandey08 abhinavmpandey08

@abhay-krishna abhay-krishna

Labels
approved lgtm ok-to-test size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@thunderboltsid @eks-distro-bot @vignesh-goutham @deepakm-ntnx @abhinavmpandey08 @abhay-krishna @jaxesn