knative / client

Knative developer experience, docs, reference Knative CLI implementation
Apache License 2.0
354 stars 262 forks source link

Compile errors with Go 1.20 #1775

Closed scottmason88 closed 1 year ago

scottmason88 commented 1 year ago

Bug report

Using Go 1.20 and building from source, I'm getting several errors.

pkg/printers/tablegenerator.go:77:2: S1011: should replace loop with `columns = append(columns, handler.columnDefinitions...)` (gosimple)
    for i := range handler.columnDefinitions {
    ^
pkg/errors/factory_test.go:172:46: loopclosure: loop variable tc captured by func literal (govet)
            assert.Equal(t, IsForbiddenError(GetError(tc.Error)), tc.Forbidden)
                                                      ^
pkg/errors/factory_test.go:212:56: loopclosure: loop variable tc captured by func literal (govet)
            assert.Equal(t, api_errors.IsInternalError(GetError(tc.Error)), tc.Internal)
                                                                ^
pkg/errors/factory_test.go:245:21: loopclosure: loop variable tc captured by func literal (govet)
            assert.Assert(t, tc.ErrorType(GetError(tc.Error)))
                             ^
cmd/kn/main.go:33:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (staticcheck)
    rand.Seed(time.Now().UnixNano())
    ^
pkg/serving/service_test.go:33:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (staticcheck)
    rand.Seed(1)
    ^
pkg/serving/service_test.go:48:3: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (staticcheck)
        rand.Seed(1)
        ^
exit status 1
--- FAIL: golangci-lint failed please fix the reported errors

Steps to reproduce the problem

Install Go 1.20, and run build.sh

kn version

Version:      v1.9.0
Build Date:   2023-01-26 20:34:03
Git Revision: df40f5a3
Supported APIs:
* Serving
  - serving.knative.dev/v1 (knative-serving v1.9.0)
* Eventing
  - sources.knative.dev/v1 (knative-eventing v1.9.0)
  - eventing.knative.dev/v1 (knative-eventing v1.9.0)

I would like to start contributing to the project, and found this while attempting to build from source for the first time. I would like to be assigned the issue so I can work through the fix.

scottmason88 commented 1 year ago

I have all but the error in cmd/kn/main.go fixed, and was looking for some direction as to what the best course of action would be. The relevant code is below:

func init() {
    rand.Seed(time.Now().UnixNano())
}

Since this has been deprecated, the 'new' way to do this is rand.New(rand.NewSource(seed)) which returns a Rand object. There are probably a few ways to fix this, and I'm also probably not aware of most of them as this is my first experience with golang. As this is also my first jump into this code, I wanted to have a discussion on the best way to move forward. My two thoughts are:

  1. Use a global, and change all the calls that used this seed to use it.
  2. Have the linter ignore this

Paul Schweigert is mentoring me through this process, and suggested that I tag @rhuss and @dsimansk in this to get their opinion.

Thanks in advance!

rhuss commented 1 year ago

Hey @scottmason88 , welcome on board :-)

Actually, I'd prefer a third option: Check where rand is used and seed it locally. Afais we are using a random number only in pkg/serving/service.go when creating revision numbers. We can create a package local variable revisionNameRand holding that seeded random variable number, in this package init() method in service.go.

The tricky part Is to update the test in service_test.go as this compares random numbers with the same seed (1), but instead of seeding it, you could replace the revisionNameRand variable with a 1-seeded rand before there tests.

Does this make sense ?