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 replace directive for cluster-api in go.mod #1510

Merged
merged 1 commit into from
Apr 11, 2022
Merged

Add replace directive for cluster-api in go.mod #1510

merged 1 commit into from
Apr 11, 2022

Conversation

ditsuke
Copy link
Contributor

@ditsuke ditsuke commented Apr 7, 2022

What this PR does / why we need it:
This PR adds a replace directive for sigs.k8s.io/cluster-api, which is required because of our dependence on the cluster-api/test package which uses a relative import for cluster-api. Reference: vmware-tanzu/tanzu-framework#1590.

Currently, running go mod tidy / make modules or go get -d ./... returns (tested on go 1.16.0 and 1.17.0):

go: sigs.k8s.io/cluster-api/test@v1.1.3 requires
        sigs.k8s.io/cluster-api@v0.0.0-00010101000000-000000000000: invalid version: unknown revision 000000000000
make: *** [Makefile:236: modules] Error 1

With this patch applied, we should no longer expect this outcome.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1509

@k8s-ci-robot
Copy link
Contributor

Hi @ditsuke. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 7, 2022
@ditsuke
Copy link
Contributor Author

ditsuke commented Apr 7, 2022 •
edited

@geetikabatra call for review. Also, should I push updates made by go mod tidy here or does that belong to another PR?

@aartij17
Copy link
Contributor

aartij17 commented Apr 7, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 7, 2022
@ditsuke
Add replace directive for CAPI required by CAPI/test
6e85ef2 
This commit adds `replace` directive for `sigs.k8s.io/cluster-api`,
which is required because we use the `cluster-api/test` package which
uses a relative import for `cluster-api`.

Signed-off-by: ditsuke <ditsuke@protonmail.com>
@ditsuke
Copy link
Contributor Author

ditsuke commented Apr 8, 2022

/retest

@ditsuke
Copy link
Contributor Author

ditsuke commented Apr 8, 2022

I believe this is a flaky test?
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-vsphere/1510/pull-cluster-api-provider-vsphere-test/1512263261116960768

geetikabatra
geetikabatra reviewed Apr 8, 2022
View reviewed changes
go.mod
@@ -2,6 +2,8 @@ module sigs.k8s.io/cluster-api-provider-vsphere

go 1.17

replace sigs.k8s.io/cluster-api => sigs.k8s.io/cluster-api v1.1.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we really need this. Why can't we update our cluster-api version than doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v1.1.3 is the latest release right now. After some investigation, I think this issue was introduced when we upgraded to v1.1.3 and removed the replace directives with bf4a27f.

@ditsuke
Copy link
Contributor Author

ditsuke commented Apr 9, 2022

/retest

1 similar comment
@srm09
Copy link
Contributor

srm09 commented Apr 11, 2022

/retest

@srm09
Copy link
Contributor

srm09 commented Apr 11, 2022

@ditsuke Nice catch, the garbled version number is the CAPI dependency version from the go.mod for the cluster-api/test dependency. We need an explicit replace directive in our go.mod to provide the correct version.

@srm09
Copy link
Contributor

srm09 commented Apr 11, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: srm09

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 89c1fac into kubernetes-sigs:main Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geetikabatra geetikabatra geetikabatra left review comments

@srm09 srm09 Awaiting requested review from srm09

@yastij yastij Awaiting requested review from yastij

Assignees

@srm09 srm09

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make modules throw error with Go 1.17
5 participants
@ditsuke @k8s-ci-robot @aartij17 @srm09 @geetikabatra