NO-JIRA: Improves version handling, build robustness, discovery error handling, and bumps versions/references across the operator#107
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request explicitly references no jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 WalkthroughMakefile: switch to granular ldflags (GO_PACKAGE, SOURCE_GIT_COMMIT, BUILD_DATE, IMG_VERSION_MAJOR/MINOR) and compose GOBUILD_VERSION_ARGS. Add pkg/version for build-time version info and log it at operator startup. Update embedded manifest version labels, tolerate partial CRD discovery errors, improve FIPS detection script, update CI operand release arg, and add a known vuln exclusion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh 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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/go-fips.sh (1)
11-12: Use single quotes in trap to defer variable expansion.The double quotes cause
${fips_test_file}to expand when the trap is set rather than when it fires. While functionally equivalent here since the variable doesn't change, single quotes are the correct idiom and prevent subtle bugs during future refactoring.🔧 Suggested fix
fips_test_file=$(mktemp --suffix=.go) -trap "rm -f ${fips_test_file}" EXIT +trap 'rm -f ${fips_test_file}' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/go-fips.sh` around lines 11 - 12, The trap currently uses double quotes causing ${fips_test_file} to be expanded immediately; change the trap to use single quotes so the variable is expanded when the trap fires (update the trap that references fips_test_file to use single-quoted string to defer expansion).pkg/controller/external_secrets/controller.go (1)
319-324: Consider logging partial discovery errors for observability.When
ServerPreferredResources()returns a partial result with an error, the error is silently discarded. Logging it at a debug/info level would help diagnose intermittent API group availability issues without changing the functional behavior.🔧 Suggested improvement
resources, err := discoveryClient.ServerPreferredResources() // ServerPreferredResources() may return a partial result along with an error (e.g., when some API groups are // unavailable). Currently, any error causes an immediate return, potentially missing CRDs that were successfully discovered. if err != nil && len(resources) == 0 { return false, fmt.Errorf("failed to discover resources list: %w", err) } + if err != nil { + ctrl.Log.V(1).WithName("crd-discovery").Info("partial discovery result, some API groups may be unavailable", "error", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/external_secrets/controller.go` around lines 319 - 324, When calling discoveryClient.ServerPreferredResources(), handle the case where err != nil && len(resources) > 0 by emitting a debug/info log that includes the err value and context that the discovery result is partial (do not change the current return behavior). Add a log statement immediately after the existing call (check the variables resources and err and the call to ServerPreferredResources()) using the repository's logger (or klog if no logger exists) to record the partial discovery error for observability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/go-fips.sh`:
- Around line 11-12: The trap currently uses double quotes causing
${fips_test_file} to be expanded immediately; change the trap to use single
quotes so the variable is expanded when the trap fires (update the trap that
references fips_test_file to use single-quoted string to defer expansion).
In `@pkg/controller/external_secrets/controller.go`:
- Around line 319-324: When calling discoveryClient.ServerPreferredResources(),
handle the case where err != nil && len(resources) > 0 by emitting a debug/info
log that includes the err value and context that the discovery result is partial
(do not change the current return behavior). Add a log statement immediately
after the existing call (check the variables resources and err and the call to
ServerPreferredResources()) using the repository's logger (or klog if no logger
exists) to record the partial discovery error for observability.
Signed-off-by: Bharath B <bhb@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/external_secrets/controller.go (1)
319-324: Consider logging the error when partial results are returned.The logic correctly tolerates partial results from
ServerPreferredResources(), but whenerr != nilandresourcesis non-empty, the error is silently discarded. Logging it at a debug/info level would aid troubleshooting API group availability issues.Proposed fix
resources, err := discoveryClient.ServerPreferredResources() // ServerPreferredResources() may return a partial result along with an error (e.g., when some API groups are // unavailable). Currently, any error causes an immediate return, potentially missing CRDs that were successfully discovered. if err != nil && len(resources) == 0 { return false, fmt.Errorf("failed to discover resources list: %w", err) } + if err != nil { + ctrl.Log.V(1).WithName("crd-discovery").Info("ServerPreferredResources returned partial results", "error", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/external_secrets/controller.go` around lines 319 - 324, When ServerPreferredResources() returns both a non-nil err and a non-empty resources slice, the error is currently discarded; modify the logic around discoveryClient.ServerPreferredResources() so that when err != nil && len(resources) > 0 you log the error at debug/info level (using the package's existing logger or klog) while still proceeding to use the partial resources; keep the existing behavior of returning an error only when resources is empty (err != nil && len(resources) == 0). Ensure you reference ServerPreferredResources(), the variables resources and err, and place the log right after the call before the conditional return.hack/go-fips.sh (1)
11-12: Use single quotes in trap to defer variable expansion (shellcheck SC2064).The variable expands when the trap is set rather than when triggered. While functionally correct here since
fips_test_fileis already assigned, single quotes are more robust if the script is refactored.Proposed fix
fips_test_file=$(mktemp --suffix=.go) -trap "rm -f ${fips_test_file}" EXIT +trap 'rm -f '"'${fips_test_file}'"'' EXITOr more simply, since the variable is already set:
fips_test_file=$(mktemp --suffix=.go) -trap "rm -f ${fips_test_file}" EXIT +trap "rm -f '${fips_test_file}'" EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/go-fips.sh` around lines 11 - 12, The trap currently uses double quotes so ${fips_test_file} is expanded when the trap is defined; change the trap invocation to use single quotes (i.e., replace the trap "rm -f ${fips_test_file}" EXIT with a single-quoted form) so the fips_test_file variable is expanded only when the trap runs, making the cleanup robust to refactors (target the trap line that references fips_test_file created by mktemp).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/go-fips.sh`:
- Around line 11-12: The trap currently uses double quotes so ${fips_test_file}
is expanded when the trap is defined; change the trap invocation to use single
quotes (i.e., replace the trap "rm -f ${fips_test_file}" EXIT with a
single-quoted form) so the fips_test_file variable is expanded only when the
trap runs, making the cleanup robust to refactors (target the trap line that
references fips_test_file created by mktemp).
In `@pkg/controller/external_secrets/controller.go`:
- Around line 319-324: When ServerPreferredResources() returns both a non-nil
err and a non-empty resources slice, the error is currently discarded; modify
the logic around discoveryClient.ServerPreferredResources() so that when err !=
nil && len(resources) > 0 you log the error at debug/info level (using the
package's existing logger or klog) while still proceeding to use the partial
resources; keep the existing behavior of returning an error only when resources
is empty (err != nil && len(resources) == 0). Ensure you reference
ServerPreferredResources(), the variables resources and err, and place the log
right after the call before the conditional return.
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
|
@bharath-b-rh: all tests passed! 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
The PR has following changes: