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

feat: upgrade to Go 1.20 #239

Merged
merged 4 commits into from
Aug 31, 2023
Merged

feat: upgrade to Go 1.20 #239

merged 4 commits into from
Aug 31, 2023

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Aug 28, 2023

  • feat: upgrade to Go 1.20
  • lint: upgrade golangci-lint to v1.54.2
  • refactor: use utils from golang.org/x/exp/slices & k8s.io/utils

Resolves: #236

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #239 (6d5c580) into main (6f3b3fd) will decrease coverage by 2.59%.
The diff coverage is 75.43%.

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
- Coverage   64.79%   62.21%   -2.59%     
==========================================
  Files          33       33              
  Lines        3224     3165      -59     
==========================================
- Hits         2089     1969     -120     
- Misses        972     1015      +43     
- Partials      163      181      +18     
Flag Coverage Δ
integration 67.25% <90.32%> (-4.56%) ⬇️
unit 57.52% <57.69%> (-0.89%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1beta1 (u) ∅ <ø> (∅)
pkg/common (u) 75.90% <83.33%> (-1.09%) ⬇️
pkg/istio (u) 30.24% <10.00%> (+0.54%) ⬆️
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 33.68% <100.00%> (ø)
pkg/rlptools (u) 57.63% <100.00%> (ø)
controllers (i) 67.25% <90.32%> (-4.56%) ⬇️
Files Changed Coverage Δ
pkg/istio/envoy_filters.go 0.00% <0.00%> (ø)
pkg/rlptools/rate_limit_index.go 17.33% <ø> (ø)
pkg/rlptools/wasm_utils.go 62.81% <ø> (ø)
controllers/authpolicy_controller.go 70.31% <66.66%> (-4.69%) ⬇️
controllers/ratelimitpolicy_controller.go 63.75% <66.66%> (-12.76%) ⬇️
pkg/common/gatewayapi_utils.go 65.92% <71.42%> (ø)
controllers/kuadrant_controller.go 48.15% <90.00%> (-4.72%) ⬇️
controllers/authpolicy_auth_config.go 79.16% <100.00%> (ø)
...ntrollers/authpolicy_istio_authorization_policy.go 60.52% <100.00%> (-2.64%) ⬇️
controllers/authpolicy_status.go 94.44% <100.00%> (ø)
... and 11 more

... and 2 files with indirect coverage changes

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

looking good

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

You may also include:

FROM golang:1.19 as builder

@KevFan
Copy link
Contributor Author

KevFan commented Aug 28, 2023

You may also include:

FROM golang:1.19 as builder

This has already been updated to 1.20 at https://github.com/Kuadrant/kuadrant-operator/pull/239/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R2 🤔

@@ -6,6 +6,7 @@ import (
"reflect"

"github.com/go-logr/logr"
"golang.org/x/exp/slices"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This slice package (https://pkg.go.dev/golang.org/x/[email protected]/slices) is provided as part of Go 1.21 standard library https://pkg.go.dev/[email protected] but is not available in Go 1.20

@KevFan KevFan marked this pull request as ready for review August 29, 2023 09:10
@KevFan KevFan requested a review from a team as a code owner August 29, 2023 09:10
Comment on lines -50 to -66
func Ptr[T any](t T) *T {
return &t
}

// FetchEnv fetches the value of the environment variable with the specified key,
// or returns the default value if the variable is not found or has an empty value.
// If an error occurs during the lookup, the function returns the default value.
// The key and default value parameters must be valid strings.
func FetchEnv(key string, def string) string {
val, ok := os.LookupEnv(key)
if !ok {
return def
}

return val
}

Copy link
Contributor Author

@KevFan KevFan Aug 29, 2023

Choose a reason for hiding this comment

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

Comment on lines -92 to -102
// Contains checks if the given target string is present in the slice of strings 'slice'.
// It returns true if the target string is found in the slice, false otherwise.
func Contains[T comparable](slice []T, target T) bool {
for idx := range slice {
if slice[idx] == target {
return true
}
}
return false
}

Copy link
Contributor Author

@KevFan KevFan Aug 29, 2023

Choose a reason for hiding this comment

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

Comment on lines -170 to -190
// SliceCopy copies the elements from the input slice into the output slice, and returns the output slice.
func SliceCopy[T any](s1 []T) []T {
s2 := make([]T, len(s1))
copy(s2, s1)
return s2
}

// ReverseSlice creates a reversed copy of the input slice.
func ReverseSlice[T any](input []T) []T {
inputLen := len(input)
output := make([]T, inputLen)

for i, n := range input {
j := inputLen - i - 1

output[j] = n
}

return output
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

nice contributions!

PS: I need to learn about new features from 1.20 😝

Comment on lines -67 to -74
// GetDefaultIfNil returns the value of a pointer argument, or a default value if the pointer is nil.
func GetDefaultIfNil[T any](val *T, def T) T {
if reflect.ValueOf(val).IsNil() {
return def
}
return *val
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing as its already provided by:

Comment on lines -173 to -182
// ContainsObjectKey tells whether a contains x
func ContainsObjectKey(a []client.ObjectKey, x client.ObjectKey) bool {
for _, n := range a {
if x == n {
return true
}
}
return false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

Awesome work, @KevFan ! Thank you!

It would be nice to test one of the user guides in #233 with the changes here just in case, but the PR gets my 👍 since now.

@KevFan
Copy link
Contributor Author

KevFan commented Aug 31, 2023

@guicassolato Sure, I've ran through https://github.com/Kuadrant/kuadrant-operator/blob/main/doc/user-guides/authenticated-rl-for-app-developers.md and it's working as expected 🎉

I'm going to merge this now 👍

@KevFan KevFan merged commit 9bfb9cb into Kuadrant:main Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tool: upgrade to go 1.20
3 participants