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

✨ Update go to 1.18 #1895

Closed
wants to merge 1 commit into from
Closed

✨ Update go to 1.18 #1895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module sigs.k8s.io/controller-runtime

go 1.17
go 1.18

require (
github.com/evanphx/json-patch v4.12.0+incompatible
Expand Down
2 changes: 1 addition & 1 deletion pkg/cache/multi_namespace_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func (c *multiNamespaceCache) WaitForCacheSync(ctx context.Context) bool {
func (c *multiNamespaceCache) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error {
isNamespaced, err := objectutil.IsAPINamespaced(obj, c.Scheme, c.RESTMapper)
if err != nil {
return nil //nolint:nilerr
return nil //nolint:ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing the linters?

Copy link
Member

Choose a reason for hiding this comment

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

@Vaxuite @alvaroaleman, it shows that what is not right here is we change the liners.
However, to make C+R begin to use go 1.18, why should we not bump the modules?
I could not follow that.

Copy link
Member

@camilamacedo86 camilamacedo86 May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm with @alvaroaleman here; I'm not sure if there's an immediate reason to update the go.mod's go version unless we do that on purpose to support go 1.18 features

usually, we bump the k8s version and go modules on deps before we upgrade Kubebuilder
Why?

  • We want to ensure the same go version across the projects in the same ecosystem. Otherwise, it is hard for contributors to switch the projects.
  • We faced problems in the past based on incompatibilities, i.e. with go 1.17:

For example, with some recent changes, we can't use k8s 1.23 with older versions of Go (For ex: operator-framework/operator-sdk#5558 (comment)) - this is a vice versa case.

Then, I do not understand how controller-runtime is using go 1.18 when its go mods are not bumped with 1.18?

Copy link
Author

Choose a reason for hiding this comment

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

So when you specify the go version in the module file its a minimum version the module must run with. So with this module on go 1.17, if you run it with go 1.18 that will work fine and this does not really need changing.

Copy link
Author

Choose a reason for hiding this comment

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

What made you think that this module needed to be bumped in the first place? @camilamacedo86

Copy link
Member

Choose a reason for hiding this comment

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

The motivation here is not bump to allow:

By keeping it on 1.17 we allow consumers of CR to use Go 1.17+.
But we are using go 1.18 in the ci, see: https://kubernetes.slack.com/archives/C02MRBMN00Z/p1652178727902549?thread_ts=1652177106.531319&cid=C02MRBMN00Z

+1 this approach has my support either.
and we should do the same in Kubebuilder.

Copy link
Member

Choose a reason for hiding this comment

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

/hold

}

if !isNamespaced {
Expand Down
2 changes: 1 addition & 1 deletion pkg/envtest/crd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func (p *poller) poll() (done bool, err error) {
// TODO: Maybe the controller-runtime client should be able to do this...
resourceList, err := cs.Discovery().ServerResourcesForGroupVersion(gv.Group + "/" + gv.Version)
if err != nil {
return false, nil //nolint:nilerr
return false, nil //nolint:ignore
}

// Remove each found resource from the resources set that we are waiting for
Expand Down
6 changes: 3 additions & 3 deletions pkg/envtest/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ func (te *Environment) Start() (*rest.Config, error) {
}
} else {
apiServer := te.ControlPlane.GetAPIServer()
if len(apiServer.Args) == 0 { //nolint:staticcheck
if len(apiServer.Args) == 0 { //nolint:ignore
// pass these through separately from above in case something like
// AddUser defaults APIServer.
//
Expand All @@ -222,7 +222,7 @@ func (te *Environment) Start() (*rest.Config, error) {
// NB(directxman12): we still pass these in so that things work if the
// user manually specifies them, but in most cases we expect them to
// be nil so that we use the new .Configure() logic.
apiServer.Args = te.KubeAPIServerFlags //nolint:staticcheck
apiServer.Args = te.KubeAPIServerFlags //nolint:ignore
}
if te.ControlPlane.Etcd == nil {
te.ControlPlane.Etcd = &controlplane.Etcd{}
Expand Down Expand Up @@ -372,4 +372,4 @@ func (te *Environment) useExistingCluster() bool {
// you can use those to append your own additional arguments.
//
// Deprecated: use APIServer.Configure() instead.
var DefaultKubeAPIServerFlags = controlplane.APIServerDefaultArgs //nolint:staticcheck
var DefaultKubeAPIServerFlags = controlplane.APIServerDefaultArgs //nolint:ignore
2 changes: 1 addition & 1 deletion pkg/internal/testing/controlplane/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ func (s *APIServer) setProcessState() error {
return err
}

s.processState.Args, s.Args, err = process.TemplateAndArguments(s.Args, s.Configure(), process.TemplateDefaults{ //nolint:staticcheck
s.processState.Args, s.Args, err = process.TemplateAndArguments(s.Args, s.Configure(), process.TemplateDefaults{ //nolint:ignore
Data: s,
Defaults: s.defaultArgs(),
MinimalDefaults: map[string][]string{
Expand Down
2 changes: 1 addition & 1 deletion pkg/internal/testing/controlplane/etcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func (e *Etcd) setProcessState() error {
e.StopTimeout = e.processState.StopTimeout

var err error
e.processState.Args, e.Args, err = process.TemplateAndArguments(e.Args, e.Configure(), process.TemplateDefaults{ //nolint:staticcheck
e.processState.Args, e.Args, err = process.TemplateAndArguments(e.Args, e.Configure(), process.TemplateDefaults{ //nolint:ignore
Data: e,
Defaults: e.defaultArgs(),
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
clusterOptions.NewClient = options.NewClient
clusterOptions.ClientDisableCacheFor = options.ClientDisableCacheFor
clusterOptions.DryRunClient = options.DryRunClient
clusterOptions.EventBroadcaster = options.EventBroadcaster //nolint:staticcheck
clusterOptions.EventBroadcaster = options.EventBroadcaster //nolint:ignore
})
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion tools/setup-envtest/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module sigs.k8s.io/controller-runtime/tools/setup-envtest

go 1.17
go 1.18

require (
github.com/go-logr/logr v1.2.0
Expand Down