ROSAENG-14107: new 'hcp-dashboard' sub-command for 'osdctl rhobs'#920
ROSAENG-14107: new 'hcp-dashboard' sub-command for 'osdctl rhobs'#920Nikokolas3270 wants to merge 1 commit into
Conversation
Command takes an option argument: the dashboard to open. The dashboard can be opened in the default browser using the --browser option.
openshift-ci-robot
added
the
jira/valid-reference
|
@Nikokolas3270: This pull request references ROSAENG-14107 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 story to target the "5.0.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. |
WalkthroughThis PR introduces a new ChangesHCP Dashboard Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
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: 3
🧹 Nitpick comments (2)
cmd/rhobs/requests.go (1)
507-528: 💤 Low valueConsider validating the RHOBS cell URL format.
The function parses the hostname suffix to infer the OCM environment but doesn't validate that
rhobsCellis a well-formed URL (e.g., starts withhttps://). If a malformed URL is passed,getRhobsCellName()(line 531) may fail with a confusing error downstream.Also note that fetchers created via this path have
IsHostedClusterandisManagementClusterset tofalse, which will cause cluster-type-specific dashboards (e.g., "hosted-cluster", "management-cluster") to reject them with validation errors. This may be intentional, but consider documenting this constraint.🛡️ Optional: Add URL scheme validation
func CreateRhobsFetcherFromCell(rhobsCell string) (*RhobsFetcher, error) { + if !strings.HasPrefix(rhobsCell, "https://") { + return nil, fmt.Errorf("RHOBS cell URL must start with 'https://': %s", rhobsCell) + } + envName := "" if strings.HasSuffix(rhobsCell, ".rhobs.api.openshift.com") {🤖 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/rhobs/requests.go` around lines 507 - 528, CreateRhobsFetcherFromCell currently infers ocmEnvName from the hostname suffix but does not validate rhobsCell as a well-formed URL; update CreateRhobsFetcherFromCell to parse rhobsCell using net/url.Parse, ensure it has a non-empty Host and an allowed scheme (preferably "https"), and return a clear error like "invalid RHOBS cell URL: must be a valid URL with https scheme" when validation fails; also explicitly set the fetcher cluster-type flags (IsHostedCluster and isManagementCluster) on the returned RhobsFetcher to false (and add a short inline comment documenting that fetchers created from a RHOBS cell URL are neither hosted nor management clusters) so downstream validation errors are less confusing.cmd/rhobs/hcpDashboard.go (1)
44-44: ⚡ Quick winConsider trimming whitespace after splitting
--rhobs-cellvalues.
strings.Split(rhobsCell, ",")does not trim whitespace. If a user provides--rhobs-cell="url1, url2"(note the space after the comma), the second URL will have a leading space, potentially causing URL parsing or fetcher creation to fail with a confusing error.✂️ Proposed fix to trim whitespace
-rhobsCells = strings.Split(rhobsCell, ",") +parts := strings.Split(rhobsCell, ",") +rhobsCells = make([]string, len(parts)) +for i, part := range parts { + rhobsCells[i] = strings.TrimSpace(part) +}🤖 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/rhobs/hcpDashboard.go` at line 44, rhobsCells produced by strings.Split(rhobsCell, ",") can contain leading/trailing whitespace (e.g. "url1, url2"); change the post-split handling so each element is strings.TrimSpace'd and optionally filter out empty entries before use (i.e., after splitting rhobsCell, iterate over the resulting slice, call strings.TrimSpace on each element, and append non-empty trimmed strings back into rhobsCells) to avoid malformed URLs when creating fetchers or parsing.
🤖 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/rhobs/hcpDashboard.go`:
- Line 40: Avoid dereferencing cmd.Parent(): replace the parent-flag check with
a direct check of the shared option variable by using commonOptions.clusterId
instead of cmd.Parent().Flags().Changed("cluster-id"); update the validation in
the function containing the conditional (the code using cmd.Parent()) to return
the conflict error when commonOptions.clusterId != "" so you remove the nil
dereference risk and decouple the child command from the parent's flag
internals.
- Around line 69-77: The code redundantly calls CreateRhobsFetcher twice causing
duplicate OCM/cluster/cell resolution and repeated
osdctlConfig.EnsureConfigFile() work; refactor by extracting the shared
initialization into a new helper (e.g., CreateRhobsFetcherContext or
InitRhobsFetchDependencies) that performs OCM connection, cluster/hypershift
resolution, RHOBS cell lookup, and ensures the osdctl config once, and then
change CreateRhobsFetcher (or add NewRhobsFetcherFromContext) to accept that
shared context and only create the specific RhobsFetcher for
RhobsFetchForMetrics and RhobsFetchForLogs; update the call site in
hcpDashboard.go to call the init helper once, then create both metrics and logs
fetchers from the returned context.
In `@cmd/rhobs/requests.go`:
- Around line 540-548: In getRhobsRegionAndShard, handle the two failure cases
separately: first check if err from getRhobsCellName() is non-nil and return
that error; then if lastDashIdx == -1 return a clear error that the returned
rhobsCellName contains no dash (include the actual rhobsCellName in the
message). Reference functions/vars: getRhobsRegionAndShard, getRhobsCellName,
rhobsCellName, lastDashIdx.
---
Nitpick comments:
In `@cmd/rhobs/hcpDashboard.go`:
- Line 44: rhobsCells produced by strings.Split(rhobsCell, ",") can contain
leading/trailing whitespace (e.g. "url1, url2"); change the post-split handling
so each element is strings.TrimSpace'd and optionally filter out empty entries
before use (i.e., after splitting rhobsCell, iterate over the resulting slice,
call strings.TrimSpace on each element, and append non-empty trimmed strings
back into rhobsCells) to avoid malformed URLs when creating fetchers or parsing.
In `@cmd/rhobs/requests.go`:
- Around line 507-528: CreateRhobsFetcherFromCell currently infers ocmEnvName
from the hostname suffix but does not validate rhobsCell as a well-formed URL;
update CreateRhobsFetcherFromCell to parse rhobsCell using net/url.Parse, ensure
it has a non-empty Host and an allowed scheme (preferably "https"), and return a
clear error like "invalid RHOBS cell URL: must be a valid URL with https scheme"
when validation fails; also explicitly set the fetcher cluster-type flags
(IsHostedCluster and isManagementCluster) on the returned RhobsFetcher to false
(and add a short inline comment documenting that fetchers created from a RHOBS
cell URL are neither hosted nor management clusters) so downstream validation
errors are less confusing.
🪄 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: Enterprise
Run ID: d2336086-f12b-4620-bdae-4494d6d492bc
📒 Files selected for processing (6)
cmd/rhobs/hcpDashboard.gocmd/rhobs/requests.gocmd/rhobs/rootCmd.godocs/README.mddocs/osdctl_rhobs.mddocs/osdctl_rhobs_hcp-dashboard.md
| metricsRhobsFetcher, err = CreateRhobsFetcher(commonOptions.clusterId, RhobsFetchForMetrics, commonOptions.hiveOcmUrl) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| logsRhobsFetcher, err = CreateRhobsFetcher(commonOptions.clusterId, RhobsFetchForLogs, commonOptions.hiveOcmUrl) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if CreateRhobsFetcher performs network calls or expensive operations
# Find the implementation
ast-grep --pattern $'func CreateRhobsFetcher($$$) $$$'
# Look for HTTP calls, OCM client usage, or heavy operations
rg -n -C5 'func CreateRhobsFetcher|http\.|ocm\.|Get.*Cluster' --type=go -g '**/rhobs/**'Repository: openshift/osdctl
Length of output: 11631
Avoid redundant OCM/cluster/config initialization when creating metrics vs logs RHOBS fetchers
cmd/rhobs/hcpDashboard.go creates two RhobsFetchers back-to-back (metrics + logs) for the same commonOptions.clusterId, and each call to CreateRhobsFetcher performs expensive setup: it creates a new OCM connection, resolves the same cluster (and hypershift management cluster when applicable), resolves the RHOBS cell, and calls osdctlConfig.EnsureConfigFile() on every invocation. This duplicates network/IO work and config reload—refactor to share/memoize the common parts between the two fetchers.
🤖 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/rhobs/hcpDashboard.go` around lines 69 - 77, The code redundantly calls
CreateRhobsFetcher twice causing duplicate OCM/cluster/cell resolution and
repeated osdctlConfig.EnsureConfigFile() work; refactor by extracting the shared
initialization into a new helper (e.g., CreateRhobsFetcherContext or
InitRhobsFetchDependencies) that performs OCM connection, cluster/hypershift
resolution, RHOBS cell lookup, and ensures the osdctl config once, and then
change CreateRhobsFetcher (or add NewRhobsFetcherFromContext) to accept that
shared context and only create the specific RhobsFetcher for
RhobsFetchForMetrics and RhobsFetchForLogs; update the call site in
hcpDashboard.go to call the init helper once, then create both metrics and logs
fetchers from the returned context.
There was a problem hiding this comment.
Yeah, maybe there should be a CreateRhobsFetchers method to mutualise those accesses.
Original code was created with the idea that a cluster has only one rhobs cell... But in reality the one used for logs can differ from the one for metrics
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at the CreateRhobsFetcher implementation to understand shared vs diverging parts
ast-grep --pattern $'func CreateRhobsFetcher($$$) ($$$) {
$$$
}'
# Also check RhobsFetcher struct definition
ast-grep --pattern $'type RhobsFetcher struct {
$$$
}'
# Check what RhobsFetchForMetrics / RhobsFetchForLogs diverge on
rg -n 'RhobsFetchFor|fetchType|FetchType|metricsCell|logsCell|IsHostedCluster' --type=goLength of output: 6218
@Nikokolas3270, that context is helpful — the design makes sense since getRhobsCell diverges per usage (metrics vs logs) but everything upstream of it (OCM connection, cluster resolution, MC lookup, config reload) is purely shared work.
A CreateRhobsFetchers function would fit naturally: do all the common init once, call getRhobsCell twice (once for each usage), then return both fetchers. Rough sketch:
// CreateRhobsFetchers creates both a metrics and a logs RhobsFetcher for the
// given cluster, sharing the OCM connection and cluster-resolution work.
func CreateRhobsFetchers(clusterKey string, hiveOcmUrl string) (metricsFetcher, logsFetcher *RhobsFetcher, err error) {
ocmConn, err := ocmutils.CreateConnection()
if err != nil {
return nil, nil, err
}
defer ocmConn.Close()
cluster, err := ocmutils.GetCluster(ocmConn, clusterKey)
if err != nil {
return nil, nil, err
}
var monitoredClusterId string
isHcp := cluster.Hypershift().Enabled()
isMC := false
if isHcp {
managementCluster, err := ocmutils.GetManagementCluster(cluster.ID())
if err != nil {
return nil, nil, fmt.Errorf("failed to retrieve management cluster for cluster '%s': %v", cluster.ID(), err)
}
monitoredClusterId = managementCluster.ID()
} else {
monitoredClusterId = cluster.ID()
isMC, err = ocmutils.IsManagementCluster(cluster.ID())
if err != nil {
return nil, nil, fmt.Errorf("failed to determine if cluster '%s' is a management cluster: %v", cluster.ID(), err)
}
}
// Cells CAN differ between metrics and logs — resolve both.
metricsCell, err := getRhobsCell(monitoredClusterId, ocmConn, RhobsFetchForMetrics, hiveOcmUrl)
if err != nil {
return nil, nil, err
}
logsCell, err := getRhobsCell(monitoredClusterId, ocmConn, RhobsFetchForLogs, hiveOcmUrl)
if err != nil {
return nil, nil, err
}
// Reload config once after backplane may have mutated it.
if err = osdctlConfig.EnsureConfigFile(); err != nil {
return nil, nil, fmt.Errorf("failed to reload osdctl config: %v", err)
}
base := RhobsFetcher{
clusterId: cluster.ID(),
clusterExternalId: cluster.ExternalID(),
clusterName: cluster.Name(),
IsHostedCluster: isHcp,
isManagementCluster: isMC,
ocmEnvName: ocmutils.GetCurrentOCMEnv(ocmConn),
}
mf := base
mf.RhobsCell = metricsCell
lf := base
lf.RhobsCell = logsCell
return &mf, &lf, nil
}hcpDashboard.go lines 69-77 and cellCmd.go lines 20-25 both exhibit the same pattern, so both call sites would benefit from switching to this helper.
Would you like me to open a follow-up GitHub issue to track this refactor, or plan the code changes directly?
✏️ Learnings added
Learnt from: Nikokolas3270
Repo: openshift/osdctl PR: 920
File: cmd/rhobs/hcpDashboard.go:69-77
Timestamp: 2026-06-12T15:13:45.487Z
Learning: In openshift/osdctl cmd/rhobs, the metrics RHOBS cell and logs RHOBS cell for a given cluster can be different. Any helper that creates both a metrics and a logs RhobsFetcher (e.g., a future CreateRhobsFetchers) must call getRhobsCell separately for RhobsFetchForMetrics and RhobsFetchForLogs, even though all other initialization (OCM connection, cluster resolution, management cluster lookup, EnsureConfigFile) is shared and should be done only once.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Nikokolas3270
Repo: openshift/osdctl PR: 920
File: cmd/rhobs/hcpDashboard.go:40-40
Timestamp: 2026-06-12T15:09:42.380Z
Learning: When implementing osdctl Cobra command logic in cmd/**/*.go, do not infer whether the user explicitly set a CLI flag by checking the derived/resolved option value (e.g., commonOptions.clusterId), especially if a parent command pre-invoke hook auto-populates that value even when the flag is omitted. Instead, detect explicit flag usage via Cobra’s flag API, e.g. use `cmd.Parent().Flags().Changed("cluster-id")` (or `cmd.Flags().Changed(...)` as appropriate for the flag being checked) to determine whether `--cluster-id` was explicitly provided by the user.
| func (f *RhobsFetcher) getRhobsRegionAndShard() (string, string, error) { | ||
| rhobsCellName, err := f.getRhobsCellName() | ||
| lastDashIdx := strings.LastIndex(rhobsCellName, "-") | ||
| if err != nil || lastDashIdx == -1 { | ||
| return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: %v", err) | ||
| } | ||
|
|
||
| return rhobsCellName[:lastDashIdx], rhobsCellName[lastDashIdx+1:], nil | ||
| } |
There was a problem hiding this comment.
Improve error message when cell name has no dash.
When lastDashIdx == -1 (cell name contains no dash), err may be nil, resulting in a confusing error message: "failed to compute RHOBS cell region & shard: <nil>".
🐛 Proposed fix
func (f *RhobsFetcher) getRhobsRegionAndShard() (string, string, error) {
rhobsCellName, err := f.getRhobsCellName()
+ if err != nil {
+ return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: %v", err)
+ }
lastDashIdx := strings.LastIndex(rhobsCellName, "-")
- if err != nil || lastDashIdx == -1 {
- return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: %v", err)
+ if lastDashIdx == -1 {
+ return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: cell name '%s' does not contain a dash", rhobsCellName)
}
return rhobsCellName[:lastDashIdx], rhobsCellName[lastDashIdx+1:], 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.
| func (f *RhobsFetcher) getRhobsRegionAndShard() (string, string, error) { | |
| rhobsCellName, err := f.getRhobsCellName() | |
| lastDashIdx := strings.LastIndex(rhobsCellName, "-") | |
| if err != nil || lastDashIdx == -1 { | |
| return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: %v", err) | |
| } | |
| return rhobsCellName[:lastDashIdx], rhobsCellName[lastDashIdx+1:], nil | |
| } | |
| func (f *RhobsFetcher) getRhobsRegionAndShard() (string, string, error) { | |
| rhobsCellName, err := f.getRhobsCellName() | |
| if err != nil { | |
| return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: %v", err) | |
| } | |
| lastDashIdx := strings.LastIndex(rhobsCellName, "-") | |
| if lastDashIdx == -1 { | |
| return "", "", fmt.Errorf("failed to compute RHOBS cell region & shard: cell name '%s' does not contain a dash", rhobsCellName) | |
| } | |
| return rhobsCellName[:lastDashIdx], rhobsCellName[lastDashIdx+1:], nil | |
| } |
🤖 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/rhobs/requests.go` around lines 540 - 548, In getRhobsRegionAndShard,
handle the two failure cases separately: first check if err from
getRhobsCellName() is non-nil and return that error; then if lastDashIdx == -1
return a clear error that the returned rhobsCellName contains no dash (include
the actual rhobsCellName in the message). Reference functions/vars:
getRhobsRegionAndShard, getRhobsCellName, rhobsCellName, lastDashIdx.
|
@Nikokolas3270: 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
Command takes an option argument: the dashboard to open.
The dashboard can be opened in the default browser using the
--browseroption.Summary by CodeRabbit
Release Notes
New Features
osdctl rhobs hcp-dashboardcommand to retrieve and construct HCP Grafana dashboard URLs. Supports optional dashboard name selection, RHOBS cell specification, and direct browser opening via--browserflag.Documentation
hcp-dashboardcommand, including available dashboards and flag usage.