SREP-347: Adding 'osdctl rhobs' subcommand to interact with RHOBS#876
Draft
Nikokolas3270 wants to merge 1 commit intoopenshift:masterfrom
Draft
SREP-347: Adding 'osdctl rhobs' subcommand to interact with RHOBS#876Nikokolas3270 wants to merge 1 commit intoopenshift:masterfrom
Nikokolas3270 wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Nikokolas3270: This pull request references SREP-347 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. 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. |
WalkthroughAdds a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Nikokolas3270 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 |
Contributor
|
@Nikokolas3270: The following tests 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. |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/vault/vault_test.go (1)
37-45: These tests mirror the implementation instead of exercising it.Both test bodies rebuild the
loginArgsand stdout/stderr branches locally rather than callingsetupVaultToken()incmd/vault/vault.go. That means they can stay green even if the production logic drifts. Please extract the command-construction/output-selection into a helper and assert against that, or inject the exec factory so the tests hit the real code path.Also applies to: 123-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/vault/vault_test.go` around lines 37 - 45, The tests currently reconstruct loginArgs and stdout/stderr themselves instead of exercising setupVaultToken; refactor so the production code exposes the command-construction logic or an injectable exec factory and update tests to call setupVaultToken so they assert the real behavior. Specifically, extract the logic that builds the vault command into a helper like buildVaultLoginArgs (or accept an execCommandFactory in setupVaultToken) and have the tests call setupVaultToken (or use the injected factory) to capture the constructed exec.Command and chosen output stream, then assert on that command args and output selection instead of rebuilding loginArgs locally; apply the same change to the other test block currently mirroring lines 123-137.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/rhobs/logsCmd.go`:
- Around line 13-37: The newCmdLogs command advertises a --cluster-id flag but
never registers it and its main_ always returns nil; fix by registering the flag
in newCmdLogs (e.g., use cmd.Flags().StringVar(&clusterId, "cluster-id", "",
"cluster identifier") or equivalent) so the flag is recognized, and change main_
to return an explicit not-implemented error (e.g., return fmt.Errorf("logs
command not implemented")) until a real implementation exists; reference
newCmdLogs for flag registration and main_ for the error return.
In `@cmd/rhobs/metricsCmd.go`:
- Around line 59-66: The function retrieveMetrics should not prepend a
human-readable banner for machine-readable outputs; update retrieveMetrics to
only print the "Metrics:" banner for non-structured formats and emit the raw
metrics payload when format is JSON or CSV. Locate retrieveMetrics and the call
to getMetrics (using cmdMetricsOptions.clusterId and promExpr) and change the
printing logic to conditionally print the banner based on MetricsFormat (skip
banner for JSON/CSV), ensuring stdout contains only the raw metrics payload for
machine consumption.
---
Nitpick comments:
In `@cmd/vault/vault_test.go`:
- Around line 37-45: The tests currently reconstruct loginArgs and stdout/stderr
themselves instead of exercising setupVaultToken; refactor so the production
code exposes the command-construction logic or an injectable exec factory and
update tests to call setupVaultToken so they assert the real behavior.
Specifically, extract the logic that builds the vault command into a helper like
buildVaultLoginArgs (or accept an execCommandFactory in setupVaultToken) and
have the tests call setupVaultToken (or use the injected factory) to capture the
constructed exec.Command and chosen output stream, then assert on that command
args and output selection instead of rebuilding loginArgs locally; apply the
same change to the other test block currently mirroring lines 123-137.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b158aa44-15a4-4a81-9ef8-28bee475559b
📒 Files selected for processing (10)
cmd/cmd.gocmd/dynatrace/requests.gocmd/requester/requester.gocmd/rhobs/logsCmd.gocmd/rhobs/metricsCmd.gocmd/rhobs/requests.gocmd/rhobs/rootCmd.gocmd/rhobs/urlCmd.gocmd/vault/vault.gocmd/vault/vault_test.go
👮 Files not reviewed due to content moderation or server errors (2)
- cmd/requester/requester.go
- cmd/rhobs/requests.go
Comment on lines
+13
to
+37
| func newCmdLogs() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "logs --cluster-id <cluster-identifier>", | ||
| Short: "Fetch logs from RHOBS.next", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| var err error | ||
| if clusterId == "" { | ||
| clusterId, err = k8s.GetCurrentCluster() | ||
| if err != nil { | ||
| cmdutil.CheckErr(err) | ||
| } | ||
| } | ||
|
|
||
| err = main_(clusterId) | ||
| if err != nil { | ||
| cmdutil.CheckErr(err) | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| return cmd | ||
| } | ||
|
|
||
| func main_(clusterId string) error { | ||
| return nil |
There was a problem hiding this comment.
logs is exposed as a real command, but it has no usable implementation yet.
Line 15 advertises --cluster-id, but the flag is never registered, so osdctl rhobs logs --cluster-id ... will fail with an unknown flag. On top of that, main_() at Lines 36-37 always returns nil, so the command can also exit successfully without doing anything. Please either keep logs unregistered until it's implemented, or at least register the flag and return an explicit not implemented error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/rhobs/logsCmd.go` around lines 13 - 37, The newCmdLogs command advertises
a --cluster-id flag but never registers it and its main_ always returns nil; fix
by registering the flag in newCmdLogs (e.g., use
cmd.Flags().StringVar(&clusterId, "cluster-id", "", "cluster identifier") or
equivalent) so the flag is recognized, and change main_ to return an explicit
not-implemented error (e.g., return fmt.Errorf("logs command not implemented"))
until a real implementation exists; reference newCmdLogs for flag registration
and main_ for the error return.
Comment on lines
+59
to
+66
| func retrieveMetrics(promExpr string, format MetricsFormat) error { | ||
| metrics, err := getMetrics(cmdMetricsOptions.clusterId, promExpr, format) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to retrieve metrics: %v", err) | ||
| } | ||
|
|
||
| fmt.Println("Metrics:") | ||
| fmt.Println(metrics) |
There was a problem hiding this comment.
Don't prepend a banner to structured output.
Lines 65-66 make -o json (and -o csv) non-parseable by tools, because stdout starts with Metrics: instead of the payload. Machine-readable formats should be emitted raw.
Suggested fix
func retrieveMetrics(promExpr string, format MetricsFormat) error {
metrics, err := getMetrics(cmdMetricsOptions.clusterId, promExpr, format)
if err != nil {
return fmt.Errorf("failed to retrieve metrics: %v", err)
}
- fmt.Println("Metrics:")
+ if format == MetricsFormatJson || format == MetricsFormatCsv {
+ fmt.Println(metrics)
+ return nil
+ }
+
+ fmt.Println("Metrics:")
fmt.Println(metrics)
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func retrieveMetrics(promExpr string, format MetricsFormat) error { | |
| metrics, err := getMetrics(cmdMetricsOptions.clusterId, promExpr, format) | |
| if err != nil { | |
| return fmt.Errorf("failed to retrieve metrics: %v", err) | |
| } | |
| fmt.Println("Metrics:") | |
| fmt.Println(metrics) | |
| func retrieveMetrics(promExpr string, format MetricsFormat) error { | |
| metrics, err := getMetrics(cmdMetricsOptions.clusterId, promExpr, format) | |
| if err != nil { | |
| return fmt.Errorf("failed to retrieve metrics: %v", err) | |
| } | |
| if format == MetricsFormatJson || format == MetricsFormatCsv { | |
| fmt.Println(metrics) | |
| return nil | |
| } | |
| fmt.Println("Metrics:") | |
| fmt.Println(metrics) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/rhobs/metricsCmd.go` around lines 59 - 66, The function retrieveMetrics
should not prepend a human-readable banner for machine-readable outputs; update
retrieveMetrics to only print the "Metrics:" banner for non-structured formats
and emit the raw metrics payload when format is JSON or CSV. Locate
retrieveMetrics and the call to getMetrics (using cmdMetricsOptions.clusterId
and promExpr) and change the printing logic to conditionally print the banner
based on MetricsFormat (skip banner for JSON/CSV), ensuring stdout contains only
the raw metrics payload for machine consumption.
|
@Nikokolas3270: This pull request references SREP-347 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
cmd/dynatrace/requests.go (3)
105-118: Variable shadows package name.The local variable
requestershadows the imported packagerequester, which can lead to confusion and potential issues if you need to reference the package again within this scope.♻️ Suggested rename
- requester := requester.Requester{ + req := requester.Requester{ Method: http.MethodPost, Url: dtURL + "platform/storage/query/v1/query:execute", Data: string(payloadJSON), Headers: map[string]string{ "Content-Type": "application/json", "Authorization": "Bearer " + accessToken, }, SuccessCode: http.StatusAccepted, } var resp string for { - resp, err = requester.Send() + resp, err = req.Send()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dynatrace/requests.go` around lines 105 - 118, The local variable named requester shadows the imported requester package; rename the local variable (e.g., reqClient or dtRequester) wherever it is declared and used (the Requester struct literal and the variable passed to resp, err = requester.Send()) so you no longer mask the package name; update all references (the Requester struct instantiation, its Headers/Method/Url/Data/SuccessCode fields, and the subsequent Send() call) to use the new variable name.
161-172: Same shadowing issue withrequestervariable.Consider renaming to
reqfor consistency across all usages in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dynatrace/requests.go` around lines 161 - 172, The variable named requester shadows the package/type name requester.Requester; rename the local variable to req for consistency and to avoid shadowing: change the instantiation from "requester := requester.Requester{...}" to "req := requester.Requester{...}" and update all subsequent uses (e.g., calls to Send()) in this scope/loop to use req.Send() and any other req fields; ensure there are no leftover references to the old variable name.
205-215: Same shadowing issue withrequestervariable.Consider renaming to
reqfor consistency across all usages in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dynatrace/requests.go` around lines 205 - 215, The variable requester is shadowing the package name requester; rename the local variable requester (from the instantiation requester := requester.Requester{...}) to req (or similar) and update its usage (req.Send(), req.Method, req.Url, req.Headers, req.SuccessCode) throughout this block so you don't conflict with the requester package identifier; ensure imports and any other references in the file remain unchanged and compile.cmd/requester/requester.go (3)
62-70: Error response handling loses context when JSON unmarshal fails.If the response body is not valid JSON (e.g., HTML error page, plain text), the unmarshal error is returned without the actual response content. This makes debugging difficult.
♻️ Proposed fix to preserve response content
if resp.StatusCode != rh.SuccessCode { var respErr responseError - err = json.Unmarshal([]byte(body), &respErr) + err = json.Unmarshal(body, &respErr) if err != nil { - return "", err + // If response is not JSON, include raw body in error + return "", fmt.Errorf("request failed: %s, body: %s", resp.Status, string(body)) } return "", fmt.Errorf("request failed: %v %s", resp.Status, respErr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/requester/requester.go` around lines 62 - 70, When handling non-success responses in requester.go (the block comparing resp.StatusCode to rh.SuccessCode), the current json.Unmarshal into responseError can fail and you return that unmarshal error without preserving the raw response body; instead catch json.Unmarshal errors and include the original body (variable body) and HTTP status in the returned error so callers see the raw response content when parsing fails (e.g., return an error that contains resp.Status and the raw body) while still attempting to use responseError when unmarshal succeeds.
29-32: Consider documenting the 600-second timeout.A 10-minute timeout is unusually long. If this is intentional (e.g., for long-running Dynatrace queries), consider adding a comment explaining why, or making it configurable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/requester/requester.go` around lines 29 - 32, The Send method currently constructs an http.Client with a hardcoded 600-second timeout (in func Send on type Requester), which is unusually long; either document why 10 minutes is required with an inline comment above the client creation (e.g., because Dynatrace queries may be long-running) or make the timeout configurable by replacing the literal with a Requester field or configuration value (e.g., rh.Timeout or a command-line/env flag) so callers can control it and add a brief comment describing the default behavior.
17-19:responseErrorfield naming may be misleading.The field is named
Recordsbut maps to JSON key"error". Consider renaming toErrorfor clarity.♻️ Proposed rename
type responseError struct { - Records json.RawMessage `json:"error"` + Error json.RawMessage `json:"error"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/requester/requester.go` around lines 17 - 19, The struct field responseError.Records is misleading because it maps to the JSON key "error"; rename the field to Error (i.e., change responseError.Records -> responseError.Error) and keep the json tag `json:"error"`, then update all usages/readers of responseError that reference Records to use Error instead (search for responseError, Records, and any unmarshalling sites to update variable access).cmd/vault/vault.go (2)
113-113: Returning pointer to map is unnecessary.Maps in Go are already reference types. Returning
*map[string]stringadds indirection without benefit and requires callers to dereference (e.g.,(*secretData)["key"]at lines 149, 153).♻️ Return map directly
-func GetSecretFromVault(vaultRef VaultRef) (*map[string]string, error) { +func GetSecretFromVault(vaultRef VaultRef) (map[string]string, error) { err := setupVaultToken(vaultRef.Addr) if err != nil { - return nil, err + return nil, err } // ... rest of function ... - return &formattedOutput.Data.Data, nil + return formattedOutput.Data.Data, nil }And update
GetCredsFromVault:- clientID, ok := (*secretData)["client_id"] + clientID, ok := secretData["client_id"] // ... - clientSecret, ok := (*secretData)["client_secret"] + clientSecret, ok := secretData["client_secret"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/vault/vault.go` at line 113, Change GetSecretFromVault to return a map[string]string (not *map[string]string) and update any functions that call it (e.g., GetCredsFromVault) to expect and pass a map directly; remove pointer indirection and replace dereferences like (*secretData)["key"] with secretData["key"]. Ensure signatures for GetSecretFromVault and GetCredsFromVault are updated consistently and update all callers to stop dereferencing the map pointer.
113-136: RedundantVAULT_ADDRenvironment variable setting.
setupVaultToken(vaultRef.Addr)already setsVAULT_ADDRat line 42. The secondos.Setenvcall at line 119 is unnecessary.♻️ Remove redundant Setenv
func GetSecretFromVault(vaultRef VaultRef) (*map[string]string, error) { err := setupVaultToken(vaultRef.Addr) if err != nil { return nil, err } - err = os.Setenv("VAULT_ADDR", vaultRef.Addr) - if err != nil { - return nil, fmt.Errorf("error setting environment variable: %v", err) - } - kvGetCommand := exec.Command("vault", "kv", "get", "-format=json", vaultRef.Path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/vault/vault.go` around lines 113 - 136, GetSecretFromVault redundantly calls os.Setenv("VAULT_ADDR", vaultRef.Addr) after setupVaultToken already sets VAULT_ADDR; remove the redundant os.Setenv call and its error handling from GetSecretFromVault so the function just relies on setupVaultToken(vaultRef.Addr) to set the env var, keeping the rest of GetSecretFromVault (exec.Command, json.Unmarshal, return) unchanged and ensure setupVaultToken remains responsible for VAULT_ADDR.cmd/rhobs/requests.go (2)
143-152: Same variable shadowing as in dynatrace/requests.go.The local variable
requestershadows the imported package name.♻️ Suggested rename
- requester := requester.Requester{ + req := requester.Requester{ Method: http.MethodGet, Url: "https://" + rhobsCell + "/api/metrics/v1/hcp/api/v1/query?" + queryData, // ... } - resp, err := requester.Send() + resp, err := req.Send()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/rhobs/requests.go` around lines 143 - 152, The local variable named requester shadows the imported package requester; rename the local variable (for example to req, reqBuilder, or requesterCfg) wherever it's declared and used so it no longer collides with the package name, e.g., change the variable assigned from requester.Requester{ ... } and all subsequent references to that variable (including fields Method, Url, Headers, SuccessCode) to the new name while leaving the imported package identifier requester unchanged.
238-261: Usefmt.Printfinstead offmt.Print(fmt.Sprintf(...)).Multiple lines use redundant
fmt.Sprintfcalls insidefmt.Print. This is flagged by staticcheck (S1038).♻️ Proposed fix
// Header - fmt.Print("|") + fmt.Print("|") for _, column := range columns { - fmt.Print(fmt.Sprintf(" %-*s |", column.width, column.name)) + fmt.Printf(" %-*s |", column.width, column.name) } // ... for _, metric := range metrics { // ... - fmt.Print(fmt.Sprintf("| %*s | %*s |", columns[0].width, time, columns[1].width, value)) + fmt.Printf("| %*s | %*s |", columns[0].width, time, columns[1].width, value) for _, column := range columns[2:] { labelValue := metric.Metric[column.name] - fmt.Print(fmt.Sprintf(" %*s |", column.width, labelValue)) + fmt.Printf(" %*s |", column.width, labelValue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/rhobs/requests.go` around lines 238 - 261, Replace redundant fmt.Print(fmt.Sprintf(...)) calls with direct fmt.Printf or fmt.Println where appropriate: use fmt.Printf for formatted strings (e.g., the header row, each metric row, and label columns) and keep fmt.Println for newline-only prints; update the blocks that render the table (references: variables columns, metrics, separatorLine and the loop over metric.Value and columns[2:]) to call fmt.Printf with the same format verbs and arguments instead of wrapping fmt.Sprintf inside fmt.Print.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/rhobs/requests.go`:
- Around line 84-87: The call to hiveClient.List is discarding its returned
error; capture and check the error from hiveClient.List(...) (e.g., assign to
err using err = hiveClient.List(...) or err := hiveClient.List(...) as
appropriate) and handle it the same way as the previous error check so that
failures listing clusterDeployments are detected and returned (referencing
hiveClient.List, clusterDeployments, clusterSelector and the existing fmt.Errorf
return).
- Around line 264-287: printMetricsAsCsv currently ignores errors from
csv.NewWriter's writer.Write and writer.Flush which can mask I/O failures;
update printMetricsAsCsv to return an error, check and propagate the error
returned by each writer.Write call (for header and each row) and the final
writer.Flush call, and update the caller at the previous call site accordingly;
locate the function printMetricsAsCsv and the loops that call writer.Write and
the final writer.Flush to add proper error handling and return propagation (also
ensure csv.NewWriter and getTableColumns usage remains unchanged).
---
Nitpick comments:
In `@cmd/dynatrace/requests.go`:
- Around line 105-118: The local variable named requester shadows the imported
requester package; rename the local variable (e.g., reqClient or dtRequester)
wherever it is declared and used (the Requester struct literal and the variable
passed to resp, err = requester.Send()) so you no longer mask the package name;
update all references (the Requester struct instantiation, its
Headers/Method/Url/Data/SuccessCode fields, and the subsequent Send() call) to
use the new variable name.
- Around line 161-172: The variable named requester shadows the package/type
name requester.Requester; rename the local variable to req for consistency and
to avoid shadowing: change the instantiation from "requester :=
requester.Requester{...}" to "req := requester.Requester{...}" and update all
subsequent uses (e.g., calls to Send()) in this scope/loop to use req.Send() and
any other req fields; ensure there are no leftover references to the old
variable name.
- Around line 205-215: The variable requester is shadowing the package name
requester; rename the local variable requester (from the instantiation requester
:= requester.Requester{...}) to req (or similar) and update its usage
(req.Send(), req.Method, req.Url, req.Headers, req.SuccessCode) throughout this
block so you don't conflict with the requester package identifier; ensure
imports and any other references in the file remain unchanged and compile.
In `@cmd/requester/requester.go`:
- Around line 62-70: When handling non-success responses in requester.go (the
block comparing resp.StatusCode to rh.SuccessCode), the current json.Unmarshal
into responseError can fail and you return that unmarshal error without
preserving the raw response body; instead catch json.Unmarshal errors and
include the original body (variable body) and HTTP status in the returned error
so callers see the raw response content when parsing fails (e.g., return an
error that contains resp.Status and the raw body) while still attempting to use
responseError when unmarshal succeeds.
- Around line 29-32: The Send method currently constructs an http.Client with a
hardcoded 600-second timeout (in func Send on type Requester), which is
unusually long; either document why 10 minutes is required with an inline
comment above the client creation (e.g., because Dynatrace queries may be
long-running) or make the timeout configurable by replacing the literal with a
Requester field or configuration value (e.g., rh.Timeout or a command-line/env
flag) so callers can control it and add a brief comment describing the default
behavior.
- Around line 17-19: The struct field responseError.Records is misleading
because it maps to the JSON key "error"; rename the field to Error (i.e., change
responseError.Records -> responseError.Error) and keep the json tag
`json:"error"`, then update all usages/readers of responseError that reference
Records to use Error instead (search for responseError, Records, and any
unmarshalling sites to update variable access).
In `@cmd/rhobs/requests.go`:
- Around line 143-152: The local variable named requester shadows the imported
package requester; rename the local variable (for example to req, reqBuilder, or
requesterCfg) wherever it's declared and used so it no longer collides with the
package name, e.g., change the variable assigned from requester.Requester{ ... }
and all subsequent references to that variable (including fields Method, Url,
Headers, SuccessCode) to the new name while leaving the imported package
identifier requester unchanged.
- Around line 238-261: Replace redundant fmt.Print(fmt.Sprintf(...)) calls with
direct fmt.Printf or fmt.Println where appropriate: use fmt.Printf for formatted
strings (e.g., the header row, each metric row, and label columns) and keep
fmt.Println for newline-only prints; update the blocks that render the table
(references: variables columns, metrics, separatorLine and the loop over
metric.Value and columns[2:]) to call fmt.Printf with the same format verbs and
arguments instead of wrapping fmt.Sprintf inside fmt.Print.
In `@cmd/vault/vault.go`:
- Line 113: Change GetSecretFromVault to return a map[string]string (not
*map[string]string) and update any functions that call it (e.g.,
GetCredsFromVault) to expect and pass a map directly; remove pointer indirection
and replace dereferences like (*secretData)["key"] with secretData["key"].
Ensure signatures for GetSecretFromVault and GetCredsFromVault are updated
consistently and update all callers to stop dereferencing the map pointer.
- Around line 113-136: GetSecretFromVault redundantly calls
os.Setenv("VAULT_ADDR", vaultRef.Addr) after setupVaultToken already sets
VAULT_ADDR; remove the redundant os.Setenv call and its error handling from
GetSecretFromVault so the function just relies on setupVaultToken(vaultRef.Addr)
to set the env var, keeping the rest of GetSecretFromVault (exec.Command,
json.Unmarshal, return) unchanged and ensure setupVaultToken remains responsible
for VAULT_ADDR.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1da9481d-674c-4905-838e-f5d227bd7f37
📒 Files selected for processing (10)
cmd/cmd.gocmd/dynatrace/requests.gocmd/requester/requester.gocmd/rhobs/logsCmd.gocmd/rhobs/metricsCmd.gocmd/rhobs/requests.gocmd/rhobs/rootCmd.gocmd/rhobs/urlCmd.gocmd/vault/vault.gocmd/vault/vault_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/vault/vault_test.go
- cmd/cmd.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/rhobs/urlCmd.go
- cmd/rhobs/metricsCmd.go
- cmd/rhobs/logsCmd.go
- cmd/rhobs/rootCmd.go
Comment on lines
+84
to
+87
| hiveClient.List(context.TODO(), &clusterDeployments, &client.ListOptions{LabelSelector: clusterSelector}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to list cluster deployments for cluster '%s': %v", cluster.ID(), err) | ||
| } |
There was a problem hiding this comment.
Critical bug: hiveClient.List error is not captured.
The error returned by hiveClient.List() is discarded. The err checked on line 85 refers to the previous error from LabelSelectorAsSelector (line 78), not the List operation. This means List failures will go undetected, and the code will proceed with potentially uninitialized clusterDeployments.
🐛 Proposed fix
- hiveClient.List(context.TODO(), &clusterDeployments, &client.ListOptions{LabelSelector: clusterSelector})
- if err != nil {
+ err = hiveClient.List(context.TODO(), &clusterDeployments, &client.ListOptions{LabelSelector: clusterSelector})
+ if err != nil {
return "", fmt.Errorf("failed to list cluster deployments for cluster '%s': %v", cluster.ID(), err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hiveClient.List(context.TODO(), &clusterDeployments, &client.ListOptions{LabelSelector: clusterSelector}) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to list cluster deployments for cluster '%s': %v", cluster.ID(), err) | |
| } | |
| err = hiveClient.List(context.TODO(), &clusterDeployments, &client.ListOptions{LabelSelector: clusterSelector}) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to list cluster deployments for cluster '%s': %v", cluster.ID(), err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 84-84: Error return value of hiveClient.List is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/rhobs/requests.go` around lines 84 - 87, The call to hiveClient.List is
discarding its returned error; capture and check the error from
hiveClient.List(...) (e.g., assign to err using err = hiveClient.List(...) or
err := hiveClient.List(...) as appropriate) and handle it the same way as the
previous error check so that failures listing clusterDeployments are detected
and returned (referencing hiveClient.List, clusterDeployments, clusterSelector
and the existing fmt.Errorf return).
Comment on lines
+264
to
+287
| func printMetricsAsCsv(metrics []rhobsMetric) { | ||
| columns := getTableColumns(metrics) | ||
|
|
||
| writer := csv.NewWriter(os.Stdout) | ||
|
|
||
| header := []string{} | ||
| for _, column := range columns { | ||
| header = append(header, column.name) | ||
| } | ||
| writer.Write(header) | ||
|
|
||
| for _, metric := range metrics { | ||
| if len(metric.Value) < 2 { | ||
| continue | ||
| } | ||
| row := []string{fmt.Sprintf("%.3f", metric.Value[0]), fmt.Sprintf("%s", metric.Value[1])} | ||
| for _, column := range columns[2:] { | ||
| row = append(row, metric.Metric[column.name]) | ||
| } | ||
| writer.Write(row) | ||
| } | ||
|
|
||
| writer.Flush() | ||
| } |
There was a problem hiding this comment.
CSV writer errors are not checked.
writer.Write() can fail (e.g., due to I/O errors). Consider checking return values for robustness, especially for the data rows.
🛡️ Proposed fix
func printMetricsAsCsv(metrics []rhobsMetric) {
+func printMetricsAsCsv(metrics []rhobsMetric) error {
columns := getTableColumns(metrics)
writer := csv.NewWriter(os.Stdout)
header := []string{}
for _, column := range columns {
header = append(header, column.name)
}
- writer.Write(header)
+ if err := writer.Write(header); err != nil {
+ return fmt.Errorf("failed to write CSV header: %v", err)
+ }
for _, metric := range metrics {
if len(metric.Value) < 2 {
continue
}
row := []string{fmt.Sprintf("%.3f", metric.Value[0]), fmt.Sprintf("%s", metric.Value[1])}
for _, column := range columns[2:] {
row = append(row, metric.Metric[column.name])
}
- writer.Write(row)
+ if err := writer.Write(row); err != nil {
+ return fmt.Errorf("failed to write CSV row: %v", err)
+ }
}
writer.Flush()
+ return writer.Error()
}Note: This requires updating the call site at line 173 and the function signature.
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 273-273: Error return value of writer.Write is not checked
(errcheck)
[error] 283-283: Error return value of writer.Write is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/rhobs/requests.go` around lines 264 - 287, printMetricsAsCsv currently
ignores errors from csv.NewWriter's writer.Write and writer.Flush which can mask
I/O failures; update printMetricsAsCsv to return an error, check and propagate
the error returned by each writer.Write call (for header and each row) and the
final writer.Flush call, and update the caller at the previous call site
accordingly; locate the function printMetricsAsCsv and the loops that call
writer.Write and the final writer.Flush to add proper error handling and return
propagation (also ensure csv.NewWriter and getTableColumns usage remains
unchanged).
petrkotas
reviewed
Apr 14, 2026
| versionCheckCmd := exec.Command("vault", "version") | ||
|
|
||
| versionCheckCmd.Stdout = os.Stdout | ||
| versionCheckCmd.Stdout = os.Stderr |
petrkotas
reviewed
Apr 14, 2026
|
|
||
| func newCmdLogs() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "logs --cluster-id <cluster-identifier>", |
Member
There was a problem hiding this comment.
Would there be an option to add simple filtering? something like --since=timestamp --untill timestamp --level ERROR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sub-subcommands which are already ready for review:
cell: give the RHOBS cell for a given or for the current clustermetrics: give the metrics for a given or for the current cluster - currently only the JSON output format is available... working on some more human friendly formatsSummary by CodeRabbit
cell,logs, andmetricssubcommands for observability queries--cluster-idflag to specify cluster for RHOBS operations