ROSAENG-61053 | chore: bump Go to 1.26.3#3321
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (30)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (22)
📝 WalkthroughWalkthroughThis PR updates Go version references in module files, Dockerfiles, and Renovate config, and removes one Renovate package rule. It also refactors command and helper code to use Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Hi @olucasfreitas this might be a good opportunity to also run the go fix command wdyt |
|
@amandahla agreed, will do it here and ping you again |
|
@amandahla added in I ran
I left the PR as draft until the paired release PR lands and Prow can pick up the Go 1.26 builder. |
olucasfreitas
force-pushed
the
ROSAENG-61053-go126
branch
from
8629526 to
9e73470
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/create/idp/cmd.go`:
- Around line 447-461: The IDP validation messages in ValidateIdpName and
getMappingMethod no longer match the existing e2e assertions, so align the
returned error strings with the expected wording. Update the cluster-admin
reserve message in ValidateIdpName and the mapping method validation message in
getMappingMethod to use the same capitalization and punctuation as the tests, or
adjust the corresponding e2e assertions to match the new strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c734004f-91cb-47af-a877-4371e66bae88
📒 Files selected for processing (19)
cmd/create/iamserviceaccount/cmd.gocmd/create/idp/cmd.gocmd/create/idp/htpasswd.gocmd/edit/ingress/flags.gocmd/list/accessrequests/cmd.gocmd/list/imagemirrors/cmd.gocmd/list/imagemirrors/cmd_test.gopkg/fedramp/config.gopkg/helper/url/helpers.gopkg/iamserviceaccount/helpers.gopkg/input/helper.gopkg/kubeletconfig/output.gopkg/logforwarding/helpers.gopkg/logforwarding/output.gopkg/logging/aws_logger.gopkg/logging/round_tripper.gopkg/network/helper.gopkg/output/reporter.gopkg/reporter/reporter.go
✅ Files skipped from review due to trivial changes (5)
- pkg/logforwarding/output.go
- cmd/list/imagemirrors/cmd_test.go
- cmd/create/idp/htpasswd.go
- pkg/logging/round_tripper.go
- cmd/create/iamserviceaccount/cmd.go
| func ValidateIdpName(idpName any) error { | ||
|
|
||
| name, ok := idpName.(string) | ||
|
|
||
| if !ok { | ||
| return fmt.Errorf("Invalid type for identity provider name. Expected a string, got %T", idpName) | ||
| return fmt.Errorf("invalid type for identity provider name: expected a string, got %T", idpName) | ||
| } | ||
|
|
||
| if !idRE.MatchString(name) { | ||
| return fmt.Errorf("Invalid identifier '%s' for 'name'", idpName) | ||
| return fmt.Errorf("invalid identifier '%s' for 'name'", idpName) | ||
| } | ||
|
|
||
| if strings.EqualFold(name, "cluster-admin") { | ||
| return fmt.Errorf("The name \"cluster-admin\" is reserved for admin user IDP") | ||
| return fmt.Errorf("the name \"cluster-admin\" is reserved for admin user IDP") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if e2e tests were updated to match new error wording.
rg -n "reserved for admin user IDP|Expected a valid mapping method" tests/e2e/Repository: openshift/rosa
Length of output: 450
Keep the IDP error strings aligned with the existing e2e assertions.
ValidateIdpName now returns the name "cluster-admin" is reserved for admin user IDP, and getMappingMethod now says expected a valid mapping method; options are %s. Existing e2e tests still look for The name "cluster-admin" is reserved for admin user IDP and Expected a valid mapping method. Options are [add claim generate lookup], so this wording change will break those checks unless the tests are updated too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/create/idp/cmd.go` around lines 447 - 461, The IDP validation messages in
ValidateIdpName and getMappingMethod no longer match the existing e2e
assertions, so align the returned error strings with the expected wording.
Update the cluster-admin reserve message in ValidateIdpName and the mapping
method validation message in getMappingMethod to use the same capitalization and
punctuation as the tests, or adjust the corresponding e2e assertions to match
the new strings.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "reserved for admin user IDP|Expected a valid mapping method|expected a valid mapping method" -C2 --iglob '*.go' .Length of output: 3759
Confirmed: tests/e2e/test_rosacli_idp.go (Lines 514 and 815) still asserts the old wording (Expected a valid mapping method. Options are [...] and The name "cluster-admin" is reserved for admin user IDP), which will break once cmd/create/idp/cmd.go emits the new lower-case strings. This needs to be fixed in one of these files before merging.
@amandahla Confirmed — the e2e tests still expect the old wording:
tests/e2e/test_rosacli_idp.goLine 514:"Failed to create IDP for cluster '%s': Expected a valid mapping method. Options are [add claim generate lookup]"tests/e2e/test_rosacli_idp.goLine 815:Expect(textData).Should(ContainSubstring("The name \"cluster-admin\" is reserved for admin user IDP"))
Since cmd/create/idp/cmd.go now returns:
"expected a valid mapping method; options are %s"(Line 553)"the name \"cluster-admin\" is reserved for admin user IDP"(Line 460)
these e2e assertions will fail unless updated. You'll need to update the two tests/e2e/test_rosacli_idp.go assertions to match the new lower-case wording (or revert the wording change in cmd.go if you prefer to keep the old strings for consistency with e2e expectations).
🧬 Code Graph Analysis Results
Possibly relevant snippet(s)
pkg/output/reporter.go (lines 39-45)
func (r *StructuredReporter) Errorf(format string, args ...any) error {
err := fmt.Errorf(format, args...)
if !PrintError(err) {
return r.inner.Errorf(format, args...)
}
return err
}pkg/output/reporter.go (lines 47-51)
func (r *StructuredReporter) Warnf(format string, args ...any) {
if !PrintWarn(fmt.Errorf(format, args...)) {
r.inner.Warnf(format, args...)
}
}pkg/output/reporter.go (lines 53-55)
func (r *StructuredReporter) Debugf(format string, args ...any) {
r.inner.Debugf(format, args...)
}pkg/output/reporter.go (lines 57-59)
func (r *StructuredReporter) Infof(format string, args ...any) {
r.inner.Infof(format, args...)
}pkg/reporter/reporter.go (lines 51-56)
func (r *Object) Debugf(format string, args ...any) {
if !debug.Enabled() {
return
}
r.Infof(format, args...)
}pkg/reporter/reporter.go (lines 59-66)
func (r *Object) Infof(format string, args ...any) {
message := fmt.Sprintf(format, args...)
if color.UseColor() {
_, _ = fmt.Fprintf(os.Stdout, "%s%s\n", infoColorPrefix, message)
} else {
_, _ = fmt.Fprintf(os.Stdout, "%s%s\n", infoPrefix, message)
}
}pkg/reporter/reporter.go (lines 69-76)
func (r *Object) Warnf(format string, args ...any) {
message := fmt.Sprintf(format, args...)
if color.UseColor() {
_, _ = fmt.Fprintf(os.Stderr, "%s%s\n", warnColorPrefix, message)
} else {
_, _ = fmt.Fprintf(os.Stderr, "%s%s\n", warnPrefix, message)
}
}pkg/reporter/reporter.go (lines 83-91)
func (r *Object) Errorf(format string, args ...any) error {
message := fmt.Sprintf(format, args...)
if color.UseColor() {
_, _ = fmt.Fprintf(os.Stderr, "%s%s\n", errorColorPrefix, message)
} else {
_, _ = fmt.Fprintf(os.Stderr, "%s%s\n", errorPrefix, message)
}
return errors.New(message)
}
|
@amandahla ready for review |
|
@olucasfreitas please check the failures, you are facing same error that I got in my PR, 1.26.4 is not available in all images. |
openshift-ci
Bot
added
the
needs-rebase
olucasfreitas
changed the title
Update the ROSA CLI toolchain from Go 1.25.8 to Go 1.26.4 across go.mod, Dockerfiles, and Renovate configuration. Remove the k8s.io/apimachinery < 0.36.0 version pin that was only necessary while the repo was on Go 1.25. Run go fix as part of the Go 1.26 upgrade and keep the verified subset of mechanical updates that passes the repo's format, lint, changed-lines coverage, and unit/integration test gates. Paired with: openshift/release#81358
Follow the 1.26.3 rollout pattern used in terraform-provider-rhcs and move the ROSA repo off 1.26.4, which is not available in all required images yet. Align go.mod, Dockerfiles, Renovate, and the committed bingo tool modules to the 1.26.3 baseline.
olucasfreitas
force-pushed
the
ROSAENG-61053-go126
branch
from
aee8afd to
23ac5e9
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@amandahla updated to the This branch now:
Local |
|
/test lint |
|
@olucasfreitas: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
Summary
go.mod, all Dockerfiles, Renovate config, and the committed Bingo tool modulesgo fix ./...mechanical cleanup that already passes the repo's local gates on this branchTest plan
GOTOOLCHAIN=go1.26.3 go mod tidyGOTOOLCHAIN=go1.26.3 go mod downloadGOTOOLCHAIN=go1.26.3 go mod verifyGOTOOLCHAIN=go1.26.3 go fix ./...make generatemake pre-push-checksRelated
Summary by CodeRabbit