Bumping to go1.23.
#1285
Bumping to go1.23.
#1285
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ybettan 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 |
WalkthroughThe pull request involves updating the Go version and toolchain in the project, along with modifications to error handling in test cases. The changes include upgrading the Go version from 1.21 to 1.23.0 in the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/controllers/nmc_reconciler_test.go (1)
387-390: Consider fixing the typo in the local variable name.There is a slight spelling discrepancy in the variable
errorMeassge; consider renaming it toerrorMessageto enhance accuracy and readability.-const ( - errorMeassge = "some error" - mod0Name = "mod0" - mod1Name = "mod1" - mod2Name = "mod2" -) +const ( + errorMessage = "some error" + mod0Name = "mod0" + mod1Name = "mod1" + mod2Name = "mod2" +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)go.mod(1 hunks)internal/controllers/nmc_reconciler_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🔇 Additional comments (1)
Makefile (1)
246-246: Validate the new golangci-lint version.Upgrading
golangci-lintfrom v1.53.1 to v1.62.2 can introduce new linters or changed default behaviors. Ensure that your lint configuration is compatible and that it yields the expected results without introducing unwanted warnings.
|
Required openshift/release#60269 to be merged first. |
|
/retest |
|
This PR should fix the linter job. |
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Dockerfile.kmm-operator-build (1)
Line range hint
8-13: Consider pinning kubectl version for reproducible builds.Currently, kubectl is installed using the latest stable version. Consider pinning to a specific version for reproducible builds.
-RUN curl -L "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" -o /tmp/kubectl +RUN curl -L "https://dl.k8s.io/release/v1.28.0/bin/linux/amd64/kubectl" -o /tmp/kubectl
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile(2 hunks)Dockerfile.kmm-operator-build(1 hunks)Dockerfile.webhook(2 hunks)Dockerfile.worker(2 hunks)Makefile(1 hunks)go.mod(1 hunks)internal/controllers/nmc_reconciler_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- go.mod
- internal/controllers/nmc_reconciler_test.go
🔇 Additional comments (5)
Dockerfile.kmm-operator-build (1)
1-1: Verify Go 1.23 compatibility with existing codebase.The upgrade from Go 1.21 to Go 1.23 is significant. Ensure all dependencies and code patterns are compatible with Go 1.23.
Run this script to check for potential compatibility issues:
✅ Verification successful
Let me gather more information about Go version compatibility.
Based on the analysis of the codebase and go.mod file, I can now provide a final response:
Go 1.23 upgrade is compatible with the codebase
The codebase is already configured to use Go 1.23.0 with toolchain go1.23.4 as specified in go.mod. The code analysis shows:
- No usage of deprecated features or breaking changes from Go 1.21 to 1.23
- Context usage follows standard patterns
- No direct syscall usage in application code (only in vendor dependencies)
- No time.Duration precision-sensitive code that would be affected by Go 1.22+ changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for deprecated features or breaking changes that might affect the codebase # Search for context.Context usage (changed in 1.22) echo "Checking context.Context usage..." rg --type go "context\.Context|ctx context\." # Check for any direct syscall usage (some syscall changes in 1.23) echo "Checking syscall usage..." rg --type go "syscall\." # Check for time.Duration conversions (precision changes in 1.22) echo "Checking time.Duration usage..." rg --type go "time\.Duration|time\.Now\(\)"Length of output: 85717
Script:
#!/bin/bash # Check for go.mod to verify Go version requirements echo "Checking go.mod for version requirements..." cat go.mod # Check for any usage of deprecated features in Go 1.23 echo -e "\nChecking for potential compatibility issues..." # Check for unsafe pointer conversions rg --type go "unsafe\.Pointer" # Check for any direct reflect usage that might be affected rg --type go "reflect\.(Value|Type|Kind)" # Check for any net/http usage that might be affected rg --type go "http\.(Client|Transport|Server)"Length of output: 85540
Dockerfile.webhook (2)
2-2: Consider security implications of switching from UBI go-toolset.The change from
registry.access.redhat.com/ubi9/go-toolset:1.21togolang:1.23moves away from Red Hat's supported and security-hardened image. While this might be necessary for Go 1.23, consider the following:
- Loss of Red Hat's security patches and support
- Different default user and permission model
- Different set of pre-installed tools and certificates
30-30: Verify binary path change across all build configurations.The binary path change from
/opt/app-root/srcto/goneeds to be consistent across all build configurations.✅ Verification successful
Binary path change is consistent across build configurations
The verification shows that the binary path change is consistent. Only two Dockerfile configurations exist that use the
/gopath:
Dockerfile.webhook:/go/webhook-serverDockerfile.worker:/go/workerThere are no remaining references to the old path
/opt/app-root/srcin any of the build configurations. The other matches for/goin the output are unrelated as they refer to Go package imports, documentation links, and vendor files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the old path echo "Checking for old path references..." rg "/opt/app-root/src" # Check for consistency in new path usage echo "Checking new path usage..." rg "/go/\w+"Length of output: 8645
Dockerfile (1)
35-35: LGTM! Good security practices in place.The implementation maintains good security practices:
- Uses UBI minimal as the final image
- Updates packages and cleans up
- Runs as non-root user (201:201)
Also applies to: 37-39
Dockerfile.worker (1)
33-33: Verify binary path consistency across Dockerfiles.The binary path has been updated to
/go/worker. Let's verify this change is consistent with other Dockerfiles in the project and that the path exists in the builder stage.✅ Verification successful
Let me try a different approach to verify the binary paths.
Let me try one more search pattern to find all Dockerfile-related files and their content.
Let me try one final search pattern to find the Dockerfile.worker specifically.
Binary path in Dockerfile.worker is consistent with the build output
Looking at the Dockerfile.worker content, I can confirm that:
- The builder stage uses
make workercommand which, according to the Makefile, builds the binary asworkerin the current directory (/goin the container)- The COPY instruction
COPY --from=builder /go/worker /usr/local/bin/workercorrectly matches the build output locationThe binary path is consistent between the build stage and the COPY instruction. No other Dockerfiles in the project copy the worker binary, as this is the dedicated Dockerfile for the worker component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in binary paths across Dockerfiles # Check binary copy paths in other Dockerfiles echo "Checking COPY commands in other Dockerfiles:" rg -B 1 -A 1 'COPY.*worker.*bin' --type dockerfile # Verify the build target in Makefile echo -e "\nChecking worker build target in Makefile:" rg 'worker:' Makefile -A 5Length of output: 669
Script:
#!/bin/bash # Search for Dockerfile patterns with different extensions echo "Checking Dockerfiles for worker binary paths:" fd -e dockerfile -e Dockerfile -x rg -B 1 -A 1 'COPY.*worker' {} echo -e "\nChecking builder stage output path:" fd -e dockerfile -e Dockerfile -x rg -B 2 'RUN.*go build.*worker' {}Length of output: 329
Script:
#!/bin/bash # List all Dockerfile-related files and show their content echo "Listing and showing content of all Dockerfile-related files:" fd -e dockerfile -e Dockerfile -x echo "=== {} ===" \; -x cat {} echo -e "\nChecking for any go build commands in Dockerfiles:" fd -e dockerfile -e Dockerfile -x rg 'go build' {}Length of output: 2298
Script:
#!/bin/bash # Search for Dockerfile.worker specifically and show its content echo "Content of Dockerfile.worker:" find . -name "Dockerfile.worker" -type f -exec cat {} \; echo -e "\nChecking if the file exists:" find . -name "Dockerfile.worker" -type f -lsLength of output: 1376
ybettan
force-pushed
the
go1.23
branch
2 times, most recently
from
6db62c4 to
c01eea7
Compare
Signed-off-by: Yoni Bettan <[email protected]>
|
/lgtm |
|
/override ci/prow/e2e-hub It is not reporting back. |
|
@ybettan: Overrode contexts on behalf of ybettan: ci/prow/e2e-hub In response to this:
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-sigs/prow repository. |
openshift-merge-bot
bot
merged commit 9256979
into
rh-ecosystem-edge:main
/assign @yevgeny-shnaidman @TomerNewman
Fixes #1283
Summary by CodeRabbit
Chores
golangci-lintversion fromv1.53.1tov1.62.21.21to1.23.0go1.22.2togo1.23.4golang:1.23Tests
NodeModulesConfigReconciler