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

bump CAPI to v1.1.0 #1137

Merged
merged 1 commit into from
Feb 11, 2022
Merged

bump CAPI to v1.1.0 #1137

merged 1 commit into from
Feb 11, 2022

Conversation

jichenjc
Copy link
Contributor

@jichenjc jichenjc commented Feb 9, 2022

What this PR does / why we need it:

  1. bump CAPI to 1.1.0
  2. update docker from 1.16 to 1.17 (otherwise lead to something like install controller-gen failed: sf.IsExported undefined controller-tools#643)
  3. update test to fit for CAPI 1.0.4->1.1.0 update (e.g logr from 0.4 to 1.2)

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 #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 9, 2022
@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

🔨 Explore the source changes: 0c720bd

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/620479283028c60007605a95

😎 Browse the preview: https://deploy-preview-1137--kubernetes-sigs-cluster-api-openstack.netlify.app

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 9, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2022
@jichenjc
Copy link
Contributor Author

jichenjc commented Feb 9, 2022

give a try on v110 per discussion in CAPI #6080

@jichenjc
Copy link
Contributor Author

jichenjc commented Feb 9, 2022

/test pull-cluster-api-provider-openstack-test

@jichenjc
Copy link
Contributor Author

jichenjc commented Feb 9, 2022

/retest

@jichenjc
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2022
@jichenjc jichenjc changed the title WIP: bump CAPI to v1.1.0 bump CAPI to v1.1.0 Feb 10, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2022
@jichenjc
Copy link
Contributor Author

/retest

@jichenjc
Copy link
Contributor Author

@mdbooth can you take a quick look? Thanks

Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned by all the noise in go.mod, and by the appearance of a second require block. However, I'm guessing you didn't write that by hand! Any idea what's going on there?

Comment on lines +30 to +118
require (
github.com/BurntSushi/toml v0.3.1 // indirect
github.com/MakeNowJust/heredoc v1.0.0 // indirect
github.com/Microsoft/go-winio v0.5.0 // indirect
github.com/PuerkitoBio/purell v1.1.1 // indirect
github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 // indirect
github.com/alessio/shellescape v1.4.1 // indirect
github.com/antlr/antlr4/runtime/Go/antlr v0.0.0-20210826220005-b48c857c3a0e // indirect
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/blang/semver v3.5.1+incompatible // indirect
github.com/cespare/xxhash/v2 v2.1.1 // indirect
github.com/containerd/containerd v1.5.9 // indirect
github.com/coredns/caddy v1.1.0 // indirect
github.com/coredns/corefile-migration v1.0.14 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/docker/distribution v2.7.1+incompatible // indirect
github.com/docker/docker v20.10.12+incompatible // indirect
github.com/docker/go-connections v0.4.0 // indirect
github.com/docker/go-units v0.4.0 // indirect
github.com/drone/envsubst/v2 v2.0.0-20210730161058-179042472c46 // indirect
github.com/evanphx/json-patch v4.12.0+incompatible // indirect
github.com/evanphx/json-patch/v5 v5.6.0 // indirect
github.com/fsnotify/fsnotify v1.5.1 // indirect
github.com/go-openapi/jsonpointer v0.19.5 // indirect
github.com/go-openapi/jsonreference v0.19.5 // indirect
github.com/go-openapi/swag v0.19.14 // indirect
github.com/gobuffalo/flect v0.2.4 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/google/cel-go v0.9.0 // indirect
github.com/google/go-cmp v0.5.6 // indirect
github.com/google/go-github/v33 v33.0.0 // indirect
github.com/google/go-querystring v1.0.0 // indirect
github.com/google/uuid v1.2.0 // indirect
github.com/googleapis/gnostic v0.5.5 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/imdario/mergo v0.3.12 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/josharian/intern v1.0.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/magiconair/properties v1.8.5 // indirect
github.com/mailru/easyjson v0.7.6 // indirect
github.com/mattn/go-isatty v0.0.14 // indirect
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
github.com/mitchellh/go-homedir v1.1.0 // indirect
github.com/mitchellh/mapstructure v1.4.2 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/nxadm/tail v1.4.8 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.0.2 // indirect
github.com/pelletier/go-toml v1.9.4 // indirect
github.com/prometheus/client_model v0.2.0 // indirect
github.com/prometheus/common v0.28.0 // indirect
github.com/prometheus/procfs v0.6.0 // indirect
github.com/sirupsen/logrus v1.8.1 // indirect
github.com/spf13/afero v1.6.0 // indirect
github.com/spf13/cast v1.4.1 // indirect
github.com/spf13/cobra v1.2.1 // indirect
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/viper v1.9.0 // indirect
github.com/stoewer/go-strcase v1.2.0 // indirect
github.com/subosito/gotenv v1.2.0 // indirect
github.com/valyala/fastjson v1.6.3 // indirect
golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect
golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8 // indirect
golang.org/x/sys v0.0.0-20211029165221-6e7872819dc8 // indirect
golang.org/x/term v0.0.0-20210615171337-6886f2dfbf5b // indirect
golang.org/x/text v0.3.7 // indirect
golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac // indirect
gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/genproto v0.0.0-20210831024726-fe130286e0e2 // indirect
google.golang.org/grpc v1.42.0 // indirect
google.golang.org/protobuf v1.27.1 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
k8s.io/apiextensions-apiserver v0.23.0 // indirect
k8s.io/apiserver v0.23.0 // indirect
k8s.io/cluster-bootstrap v0.23.0 // indirect
k8s.io/kube-openapi v0.0.0-20211115234752-e816edb12b65 // indirect
sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect
sigs.k8s.io/kind v0.11.1 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.2.0 // indirect
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea where this appeared from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing you didn't write that by hand! Any idea what's going on there?

yes, I didn't do it manually...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I think it do no harm ? I don't know why update to 1.1.0 introduce so much changes

the //indirect is not something familiar to me , will do some home work..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/kubernetes-sigs/cluster-api/blob/main/go.mod#L51
capi also has this kind of stuffs

not find the real solution yet, but seems other folks has same questions as well
https://stackoverflow.com/questions/61115111/how-to-avoid-indirect-dependencies-in-go-mod-file

Also curious why only this update (maybe go16->17?) trigger this...

Copy link
Member

Choose a reason for hiding this comment

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

I saw this happening locally when upgrading from go 1.16 to 1.17. I just checked and it's in the release notes for 1.17: https://go.dev/doc/go1.17#graph-pruning

Because the number of explicit requirements may be substantially larger in an expanded Go 1.17 go.mod file, the newly-added requirements on indirect dependencies in a go 1.17 module are maintained in a separate require block from the block containing direct dependencies.

@@ -77,7 +76,6 @@ func Test_GetOrCreateFloatingIP(t *testing.T) {
tt.expect(mockClient.EXPECT())
s := Service{
client: mockClient,
logger: logr.DiscardLogger{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check the commit msg :)

the logr 0.4=>1.2 update makes this happen :)

@@ -439,7 +438,6 @@ func Test_GetOrCreatePort(t *testing.T) {
tt.expect(mockClient.EXPECT())
s := Service{
client: mockClient,
logger: logr.DiscardLogger{},
Copy link
Contributor

Choose a reason for hiding this comment

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

And again. Is there some behaviour change in logr which means calling methods on a nil logr doesn't error?

Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

I don't understand specifically why these new requires have turned up with this change, but having looked they're all clearly required. Suspect the answer is probably go 1.17, although that's purely a guess.

Also having the indirects in a separate requires is somewhat tidier. Despite my initial surprise I think I prefer it.

Incidentally, most of these deps seem to be pulled in via sigs.k8s.io/cluster-api/test, which is a separate module in CAPI. I wonder if we should also pull our e2e tests into a separate module to minimise the blast radius. @sbueringer do you happen to know if other projects are doing this?

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, mdbooth

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

@mdbooth
Copy link
Contributor

mdbooth commented Feb 10, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2022
@sbueringer
Copy link
Member

I don't understand specifically why these new requires have turned up with this change, but having looked they're all clearly required. Suspect the answer is probably go 1.17, although that's purely a guess.

Also having the indirects in a separate requires is somewhat tidier. Despite my initial surprise I think I prefer it.

Incidentally, most of these deps seem to be pulled in via sigs.k8s.io/cluster-api/test, which is a separate module in CAPI. I wonder if we should also pull our e2e tests into a separate module to minimise the blast radius. @sbueringer do you happen to know if other projects are doing this?

/lgtm

Yes go 1.17 introduced the new "go.mod format"

Afaik only core CAPI has a separate module. There is a somewhat related suggestion on that issue: kubernetes-sigs/cluster-api#6081 (comment)

But I don't know what I personally would prefer.

@jichenjc
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2022
@jichenjc
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 1b7e8da into kubernetes-sigs:main Feb 11, 2022
@chrischdi chrischdi mentioned this pull request Apr 19, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants