Fix Grafana log links in osdctl promote by discovering namespace and SaaS file names#853
Fix Grafana log links in osdctl promote by discovering namespace and SaaS file names#853xiaoyu74 wants to merge 1 commit intoopenshift:masterfrom
Conversation
xiaoyu74
force-pushed
the
improve_e2e_test_log_links
branch
from
3f00684 to
9df57a5
Compare
|
/hold AppSRE's cloudwatch and Grafana should be the main logging entrypoint, not Tekton. Not everyone has access to tekton and the retention is very short for those jobs. |
openshift-ci
bot
added
the
do-not-merge/hold
Thanks Dustin, as discussed in the thread, I will update the PR to keep using the Grafana links and fixing the incorrect namespace mapping. |
xiaoyu74
changed the title
xiaoyu74
force-pushed
the
improve_e2e_test_log_links
branch
from
9df57a5 to
900c23a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xiaoyu74 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds pipeline-namespace discovery by parsing Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "promote CLI"
participant App as "git.AppInterface"
participant FS as "Repository files (SaaS YAML)"
participant Builder as "Grafana URL builder"
CLI->>App: request ServicesFilesMap for serviceName
App->>FS: read `services/<service>.saas.yaml`
FS-->>App: SaaS YAML content
App-->>CLI: return file content
CLI->>Builder: discoverPipelineNamespace(content)
Builder-->>Builder: parse pipelinesProvider.$ref -> pipeline filename
Builder-->>Builder: strip prefixes/suffixes -> namespace
Builder-->>CLI: namespace (or error)
CLI->>Builder: generateGrafanaLogsURL(serviceName, operator, gitHash, env, namespace?)
Builder-->>Builder: discover e2e SaaS filename (if needed)
Builder-->>CLI: Grafana URL with var-namespace, var-targetref, var-env, var-saasfilename, etc.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xiaoyu74
force-pushed
the
improve_e2e_test_log_links
branch
from
900c23a to
98274a2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/promote/saas/utils.go (1)
171-180:⚠️ Potential issue | 🔴 CriticalAdd environment-specific logic to use correct SaaS file naming convention for STAGE.
The code currently attempts to discover E2E test SaaS files for both INT and STAGE environments. However, based on AppSRE conventions, STAGE deployment pipelines use regular deployment SaaS files (
saas-<service>.yaml), while INT uses E2E test files.This explains why STAGE returns "No data" in Grafana when looking for non-existent E2E test pipelines. The
envparameter should control whether to discover E2E test SaaS names (for INT) or regular deployment SaaS names (for STAGE).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/promote/saas/utils.go` around lines 171 - 180, The code always tries to discover an E2E test SaaS name via discoverE2ETestSaasName and falls back to an E2E-style name; change this to use the env parameter to pick the correct naming convention: if env indicates INT, call discoverE2ETestSaasName(appInterface, operatorName) and fall back to fmt.Sprintf("saas-%s-e2e-test", operatorName); if env indicates STAGE, either call a regular discovery function (e.g., discoverRegularSaasName(appInterface, operatorName)) or directly set the name to the regular deployment filename fmt.Sprintf("saas-%s.yaml", operatorName) and log accordingly; update references to e2eTestSaasName and the fmt.Printf messages to reflect the branch chosen.
🧹 Nitpick comments (1)
cmd/promote/saas/utils.go (1)
96-101: Unused parameterappInterfacein function signature.The
discoverPipelineNamespacefunction acceptsappInterface git.AppInterfacebut never uses it. The function only accesses the globalServicesFilesMap. This contrasts withdiscoverE2ETestSaasNamewhich usesappInterface.GitDirectory.Consider either removing the unused parameter or refactoring to use
appInterfaceconsistently (e.g., deriving the SaaS file path fromappInterface.GitDirectoryinstead of relying on the global map).♻️ Option 1: Remove unused parameter
-func discoverPipelineNamespace(appInterface git.AppInterface, serviceName string) (string, error) { +func discoverPipelineNamespace(serviceName string) (string, error) {This would require updating all call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/promote/saas/utils.go` around lines 96 - 101, The function discoverPipelineNamespace has an unused parameter appInterface (git.AppInterface) while it only reads ServicesFilesMap; either remove the appInterface parameter from discoverPipelineNamespace and update all call sites to match, or change the implementation to derive the saasFilePath using appInterface.GitDirectory (like discoverE2ETestSaasName) instead of ServicesFilesMap; update references to the function and any callers accordingly so the signature and usage are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/promote/saas/utils.go`:
- Around line 171-180: The code always tries to discover an E2E test SaaS name
via discoverE2ETestSaasName and falls back to an E2E-style name; change this to
use the env parameter to pick the correct naming convention: if env indicates
INT, call discoverE2ETestSaasName(appInterface, operatorName) and fall back to
fmt.Sprintf("saas-%s-e2e-test", operatorName); if env indicates STAGE, either
call a regular discovery function (e.g., discoverRegularSaasName(appInterface,
operatorName)) or directly set the name to the regular deployment filename
fmt.Sprintf("saas-%s.yaml", operatorName) and log accordingly; update references
to e2eTestSaasName and the fmt.Printf messages to reflect the branch chosen.
---
Nitpick comments:
In `@cmd/promote/saas/utils.go`:
- Around line 96-101: The function discoverPipelineNamespace has an unused
parameter appInterface (git.AppInterface) while it only reads ServicesFilesMap;
either remove the appInterface parameter from discoverPipelineNamespace and
update all call sites to match, or change the implementation to derive the
saasFilePath using appInterface.GitDirectory (like discoverE2ETestSaasName)
instead of ServicesFilesMap; update references to the function and any callers
accordingly so the signature and usage are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2a208d6-d77c-4d5a-9923-eb4621baa5f6
📒 Files selected for processing (2)
cmd/promote/saas/utils.gocmd/promote/saas/utils_test.go
|
@xiaoyu74: 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/promote/saas/utils.go (1)
157-191:⚠️ Potential issue | 🟠 MajorUse the deployment SaaS name for STAGE.
Lines 172-176 still force
var-saasfilenametosaas-<operator>-e2e-testfor every env. That preserves the STAGE bug called out in the PR: most operators only run the regular deploy pipeline in STAGE, so the generated STAGE link will keep filtering to a non-existent e2e PipelineRun and return “No data”.💡 Suggested direction
- e2eTestSaasName, err := discoverE2ETestSaasName(appInterface, operatorName) + saasName, err := discoverGrafanaSaasName(appInterface, serviceName, operatorName, env) ... - url += fmt.Sprintf("&var-saasfilename=%s", e2eTestSaasName) + url += fmt.Sprintf("&var-saasfilename=%s", saasName)// INT -> resolve the e2e test SaaS YAML name // STAGE -> resolve the promoted/deploy SaaS YAML name
🤖 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/promote/saas/utils.go`:
- Around line 96-106: discoverPipelineNamespace currently reads the raw
ServicesFilesMap value which may be a directory (causing EISDIR); ensure the
function operates on the resolved SaaS YAML path by either (a) changing callers
to call GetSaasDir(servicePath) and pass that resulting YAML path into
discoverPipelineNamespace, or (b) update discoverPipelineNamespace to call
GetSaasDir on the received service path before os.ReadFile; reference the
discoverPipelineNamespace function and the GetSaasDir helper when making the
change so directory-backed services are converted to their deploy.yaml /
hypershift-deploy.yaml file before reading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d47cc62-484b-4ebb-aa5d-9a31d47fa10f
📒 Files selected for processing (2)
cmd/promote/saas/utils.gocmd/promote/saas/utils_test.go
| func discoverPipelineNamespace(appInterface git.AppInterface, serviceName string) (string, error) { | ||
| // Get the SaaS file path from the services map | ||
| saasFilePath, ok := ServicesFilesMap[serviceName] | ||
| if !ok { | ||
| return "", fmt.Errorf("saas file not found for service %s", serviceName) | ||
| } | ||
|
|
||
| // Read the SaaS YAML file | ||
| fileContent, err := os.ReadFile(saasFilePath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read SaaS file: %w", err) |
There was a problem hiding this comment.
Resolve the actual SaaS YAML before reading it.
Line 104 reads the raw ServicesFilesMap entry, but that map also contains saas-* directories; GetSaasDir only converts those into deploy.yaml / hypershift-deploy.yaml later in this file. For directory-backed services this helper will hit EISDIR, fall back to <operator>-pipelines, and keep the Grafana link wrong for the exact operators this PR is trying to fix.
🐛 Suggested direction
-func discoverPipelineNamespace(appInterface git.AppInterface, serviceName string) (string, error) {
- saasFilePath, ok := ServicesFilesMap[serviceName]
- if !ok {
- return "", fmt.Errorf("saas file not found for service %s", serviceName)
- }
+func discoverPipelineNamespace(saasFilePath string) (string, error) {// Call this with the already-resolved YAML path returned by GetSaasDir.
namespace, err := discoverPipelineNamespace(saasDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/promote/saas/utils.go` around lines 96 - 106, discoverPipelineNamespace
currently reads the raw ServicesFilesMap value which may be a directory (causing
EISDIR); ensure the function operates on the resolved SaaS YAML path by either
(a) changing callers to call GetSaasDir(servicePath) and pass that resulting
YAML path into discoverPipelineNamespace, or (b) update
discoverPipelineNamespace to call GetSaasDir on the received service path before
os.ReadFile; reference the discoverPipelineNamespace function and the GetSaasDir
helper when making the change so directory-backed services are converted to
their deploy.yaml / hypershift-deploy.yaml file before reading.
2 participants
Problem
When
osdctl promotegenerates app-interface promotion(e.g MR-174930) the Grafana dashboard links frequently show "No data" even when pipelines exist and completed successfully.Root Cause
Incorrect namespace mapping - The code used a hardcoded pattern
{operator-name}-pipelineswhich fails foroperators that use:
managed-cluster-validating-webhooks- >mcvw-pipelines(notmanaged-cluster-validating-webhooks-pipelines)backplane-api,backplane-dashboards- >backplane-pipelinescompliance-monkey- >osd-operators-pipelinesThis affects both INT and STAGE Grafana links.
What's the PR trying to resolve?
Discover the actual namespace from each operator's SaaS YAML file by reading the
pipelinesProvider.$reffieldinstead of using a hardcoded pattern.
Example for MCVW:
Parse the filename to extract the namespace: mcvw-pipelines
Changes
discoverE2ETestSaasName()function - Discovers E2E test SaaS file name from YAML (restores functionality removed in earlier PR)generateGrafanaLogsURL()function - Uses discoverPipelineNamespace() to fix namespace bugTest with a dummy MCVW promotion MR
mcvw-pipelinesnamespace.Summary by CodeRabbit
Refactor
Tests