SREP-3640: add dry run option and link to logs to output for CAD run …#846
SREP-3640: add dry run option and link to logs to output for CAD run …#846rolandmkunkel wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rolandmkunkel: This pull request references SREP-3640 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 "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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rolandmkunkel 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
rolandmkunkel
force-pushed
the
SREP-3640-add-dry-run-and-link-to-logs
branch
2 times, most recently
from
41cd305 to
29ec4ea
Compare
WalkthroughAdds a --dry-run flag and propagates dry-run state through the CAD run flow, introduces Grafana/AWS config keys and validation, replaces hard-coded CAD namespaces with constants, refactors validation using slices.Contains, and updates tests and documentation for the new behavior. Changes
Sequence Diagram(s)mermaid CLI->>Run: invoke Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@rolandmkunkel: This pull request references SREP-3640 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 "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. |
![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 (1)
cmd/cluster/cad/run_test.go (1)
125-231: Logs-link tests aren’t exercising the real URL builder.The new tests re-implement the logic (and one uses a self-fulfilling count), so they can pass even if the production URL construction changes or breaks. Consider extracting a small
buildLogsLink(...)helper inrun.goand asserting against its output here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/cad/run_test.go` around lines 125 - 231, The tests are re-implementing the URL construction logic instead of validating the real builder; add a small exported helper buildLogsLink(pipelineRunName, grafanaURL, awsAccountID string) in run.go that returns the constructed logs URL, then update TestLogsLinkGeneration and TestLogsLinkURLConstruction to call buildLogsLink(...) (instead of redoing logic), assert the returned URL contains the grafanaURL and the correct number of awsAccountID occurrences (replace the self-fulfilling strings.Repeat count with strings.Count on the returned URL), and remove the duplicated construction logic so tests exercise the production code path.
🤖 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/setup/setup.go`:
- Around line 120-127: The optional-keys loop currently skips validation for
CADGrafanaURL and CADAWSAccountID (their checks live only in the required-keys
loop), so modify the optional-keys handling to run the same validation when an
optional value is present: for CADGrafanaURL call ValidateURL(value) (when value
!= "" && value != defaultValue) and for CADAWSAccountID call
ValidateAWSAccount(value) under the same condition, and propagate/handle the err
exactly as the required-keys branch does; apply the same changes to the other
optional CAD entries noted (lines corresponding to the similar block around
137-149) so optional inputs are validated before persisting.
In `@docs/osdctl_cluster_cad_run.md`:
- Around line 20-34: The Examples block is triggering MD023 because the indented
lines starting with `#` are parsed as headings; wrap the example commands in a
fenced code block (e.g., add a ```bash before the first example and a closing
``` after the second) in docs/osdctl_cluster_cad_run.md so the content under the
Examples section is treated as code, or alternatively indent each example by 4
spaces; target the Examples block shown (the two osdctl cluster cad run
examples) to apply the fix.
In `@docs/README.md`:
- Around line 1305-1320: The CAD run Examples block is being parsed as headings
(MD023); wrap the entire example commands block in a fenced code block using
triple backticks with the language tag (e.g., ```bash) before the first example
line and a closing ``` after the last line so the indented lines are rendered as
code; locate the "Examples:" / CAD run example lines in README.md and add the
opening fence above the first "# Run a change management..." line and the
closing fence after the "--dry-run" line to preserve indentation and prevent
markdownlint warnings.
---
Nitpick comments:
In `@cmd/cluster/cad/run_test.go`:
- Around line 125-231: The tests are re-implementing the URL construction logic
instead of validating the real builder; add a small exported helper
buildLogsLink(pipelineRunName, grafanaURL, awsAccountID string) in run.go that
returns the constructed logs URL, then update TestLogsLinkGeneration and
TestLogsLinkURLConstruction to call buildLogsLink(...) (instead of redoing
logic), assert the returned URL contains the grafanaURL and the correct number
of awsAccountID occurrences (replace the self-fulfilling strings.Repeat count
with strings.Count on the returned URL), and remove the duplicated construction
logic so tests exercise the production code path.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
cmd/cluster/cad/run.gocmd/cluster/cad/run_test.gocmd/setup/setup.gocmd/setup/setup_test.godocs/README.mddocs/osdctl_cluster_cad_run.md
rolandmkunkel
force-pushed
the
SREP-3640-add-dry-run-and-link-to-logs
branch
from
29ec4ea to
4a2678e
Compare
|
@rolandmkunkel: This pull request references SREP-3640 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 "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: 1
🧹 Nitpick comments (1)
cmd/cluster/cad/run_test.go (1)
125-233: Logs link tests don’t exercise the real URL construction.Both
TestLogsLinkGenerationandTestLogsLinkURLConstructiononly check config presence or a synthetic string, so they can pass even if the actual logs link format incmd/cluster/cad/run.gobreaks. Consider extracting abuildLogsLink(...)helper and asserting on its output here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/cad/run_test.go` around lines 125 - 233, The tests currently only validate config presence and synthetic strings rather than the actual link logic; extract the URL construction from cmd/cluster/cad/run.go into a pure helper function named buildLogsLink(grafanaURL, awsAccountID, pipelineRunName string) string (or similar), update the production code to call that function, and modify TestLogsLinkGeneration and TestLogsLinkURLConstruction to call buildLogsLink(...) and assert the returned URL contains the expected grafana host, the pipelineRunName, and the expected number/occurrence of awsAccountID references and exact substrings instead of inspecting viper only.
🤖 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/setup/setup.go`:
- Line 35: The URLRegex constant and ValidateURL are too strict (only
scheme+host+optional port) and therefore reject Grafana URLs served under a
path; update the URLRegex used by ValidateURL to permit optional path segments
(e.g. allow a slash followed by valid path characters and optional
query/fragment) and adjust ValidateURL to use the new pattern (reference
symbols: URLRegex and ValidateURL) so URLs like https://example.com/grafana or
https://example.com:3000/grafana/dashboards are accepted.
---
Nitpick comments:
In `@cmd/cluster/cad/run_test.go`:
- Around line 125-233: The tests currently only validate config presence and
synthetic strings rather than the actual link logic; extract the URL
construction from cmd/cluster/cad/run.go into a pure helper function named
buildLogsLink(grafanaURL, awsAccountID, pipelineRunName string) string (or
similar), update the production code to call that function, and modify
TestLogsLinkGeneration and TestLogsLinkURLConstruction to call
buildLogsLink(...) and assert the returned URL contains the expected grafana
host, the pipelineRunName, and the expected number/occurrence of awsAccountID
references and exact substrings instead of inspecting viper only.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
cmd/cluster/cad/run.gocmd/cluster/cad/run_test.gocmd/setup/setup.gocmd/setup/setup_test.godocs/README.mddocs/osdctl_cluster_cad_run.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/setup/setup_test.go
| DtVaultPathRegex = `^[a-zA-Z0-9\-/]+$` | ||
| CloudTrailCmdListsRegex = `^\s*-\s+.*$` | ||
| GitLabTokenRegex = `^[a-zA-Z0-9]{20}$` // #nosec G101 | ||
| URLRegex = `^https?:\/\/[a-zA-Z0-9.-]+(:\d+)?$` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the relevant file
find . -name "setup.go" -path "*/cmd/setup/*" | head -5Repository: openshift/osdctl
Length of output: 80
🏁 Script executed:
# Read the setup.go file to examine lines around 35 and 269-279
cat -n ./cmd/setup/setup.go | head -100Repository: openshift/osdctl
Length of output: 3512
🏁 Script executed:
# Continue reading from line 100 to see more of the validation logic and line 269-279
cat -n ./cmd/setup/setup.go | sed -n '100,280p'Repository: openshift/osdctl
Length of output: 5523
🏁 Script executed:
# Let's check if there are any tests for URLRegex or ValidateURL
find . -name "*test.go" -o -name "*_test.go" | xargs grep -l "URLRegex\|ValidateURL" 2>/dev/nullRepository: openshift/osdctl
Length of output: 85
🏁 Script executed:
# Read the test file to see test cases for URLRegex and ValidateURL
cat -n ./cmd/setup/setup_test.goRepository: openshift/osdctl
Length of output: 9564
URL validation rejects Grafana URLs served from subpaths.
Line 35: URLRegex only permits scheme+host(+port). If Grafana is served under a path prefix (e.g., /grafana), ValidateURL will reject it and setup cannot save the config. If subpaths are expected, loosen the pattern to allow optional path segments.
✅ Minimal fix to allow optional path segments
- URLRegex = `^https?:\/\/[a-zA-Z0-9.-]+(:\d+)?$`
+ URLRegex = `^https?:\/\/[a-zA-Z0-9.-]+(:\d+)?(\/.*)?$`Also applies to: Line 269-279 (ValidateURL function)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/setup/setup.go` at line 35, The URLRegex constant and ValidateURL are too
strict (only scheme+host+optional port) and therefore reject Grafana URLs served
under a path; update the URLRegex used by ValidateURL to permit optional path
segments (e.g. allow a slash followed by valid path characters and optional
query/fragment) and adjust ValidateURL to use the new pattern (reference
symbols: URLRegex and ValidateURL) so URLs like https://example.com/grafana or
https://example.com:3000/grafana/dashboards are accepted.
|
@rolandmkunkel: 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. |
|
|
||
| var logsLink string | ||
| if grafanaURL != "" && awsAccountID != "" { | ||
| logsLink = fmt.Sprintf("%s/explore?schemaVersion=1&panes=%%7B%%22buh%%22:%%7B%%22datasource%%22:%%22P1A97A9592CB7F392%%22,%%22queries%%22:%%5B%%7B%%22id%%22:%%22%%22,%%22region%%22:%%22us-east-1%%22,%%22namespace%%22:%%22%%22,%%22refId%%22:%%22A%%22,%%22datasource%%22:%%7B%%22type%%22:%%22cloudwatch%%22,%%22uid%%22:%%22P1A97A9592CB7F392%%22%%7D,%%22queryMode%%22:%%22Logs%%22,%%22logGroups%%22:%%5B%%7B%%22arn%%22:%%22arn:aws:logs:us-east-1:%s:log-group:cads01ue1.configuration-anomaly-detection-stage:%%2A%%22,%%22name%%22:%%22cads01ue1.configuration-anomaly-detection-stage%%22,%%22accountId%%22:%%22%s%%22%%7D,%%7B%%22arn%%22:%%22arn:aws:logs:us-east-1:%s:log-group:cadp01ue1.configuration-anomaly-detection-production:%%2A%%22,%%22name%%22:%%22cadp01ue1.configuration-anomaly-detection-production%%22,%%22accountId%%22:%%22%s%%22%%7D%%5D,%%22expression%%22:%%22fields%%20message%%5Cn%%7C%%20filter%%20kubernetes.pod_name%%20like%%20%%5C%%22%s%%5C%%22%%22,%%22statsGroups%%22:%%5B%%5D%%7D%%5D,%%22range%%22:%%7B%%22from%%22:%%22now-1h%%22,%%22to%%22:%%22now%%22%%7D,%%22panelsState%%22:%%7B%%22logs%%22:%%7B%%22visualisationType%%22:%%22logs%%22%%7D%%7D%%7D%%7D&orgId=1", grafanaURL, awsAccountID, awsAccountID, awsAccountID, awsAccountID, pipelineRunName) |
There was a problem hiding this comment.
| logsLink = fmt.Sprintf("%s/explore?schemaVersion=1&panes=%%7B%%22buh%%22:%%7B%%22datasource%%22:%%22P1A97A9592CB7F392%%22,%%22queries%%22:%%5B%%7B%%22id%%22:%%22%%22,%%22region%%22:%%22us-east-1%%22,%%22namespace%%22:%%22%%22,%%22refId%%22:%%22A%%22,%%22datasource%%22:%%7B%%22type%%22:%%22cloudwatch%%22,%%22uid%%22:%%22P1A97A9592CB7F392%%22%%7D,%%22queryMode%%22:%%22Logs%%22,%%22logGroups%%22:%%5B%%7B%%22arn%%22:%%22arn:aws:logs:us-east-1:%s:log-group:cads01ue1.configuration-anomaly-detection-stage:%%2A%%22,%%22name%%22:%%22cads01ue1.configuration-anomaly-detection-stage%%22,%%22accountId%%22:%%22%s%%22%%7D,%%7B%%22arn%%22:%%22arn:aws:logs:us-east-1:%s:log-group:cadp01ue1.configuration-anomaly-detection-production:%%2A%%22,%%22name%%22:%%22cadp01ue1.configuration-anomaly-detection-production%%22,%%22accountId%%22:%%22%s%%22%%7D%%5D,%%22expression%%22:%%22fields%%20message%%5Cn%%7C%%20filter%%20kubernetes.pod_name%%20like%%20%%5C%%22%s%%5C%%22%%22,%%22statsGroups%%22:%%5B%%5D%%7D%%5D,%%22range%%22:%%7B%%22from%%22:%%22now-1h%%22,%%22to%%22:%%22now%%22%%7D,%%22panelsState%%22:%%7B%%22logs%%22:%%7B%%22visualisationType%%22:%%22logs%%22%%7D%%7D%%7D%%7D&orgId=1", grafanaURL, awsAccountID, awsAccountID, awsAccountID, awsAccountID, pipelineRunName) | |
| logsLink = fmt.Sprintf("%s/explore?schemaVersion=1&panes=%%7B%%22buh%%22:%%7B%%22datasource%%22:%%22P1A97A9592CB7F392%%22,%%22queries%%22:%%5B%%7B%%22id%%22:%%22%%22,%%22region%%22:%%22us-east-1%%22,%%22namespace%%22:%%22%%22,%%22refId%%22:%%22A%%22,%%22datasource%%22:%%7B%%22type%%22:%%22cloudwatch%%22,%%22uid%%22:%%22P1A97A9592CB7F392%%22%%7D,%%22queryMode%%22:%%22Logs%%22,%%22logGroups%%22:%%5B%%7B%%22arn%%22:%%22arn:aws:logs:us-east-1:%[2]s:log-group:cads01ue1.configuration-anomaly-detection-stage:%%2A%%22,%%22name%%22:%%22cads01ue1.configuration-anomaly-detection-stage%%22,%%22accountId%%22:%%22%[2]s%%22%%7D,%%7B%%22arn%%22:%%22arn:aws:logs:us-east-1:%[2]s:log-group:cadp01ue1.configuration-anomaly-detection-production:%%2A%%22,%%22name%%22:%%22cadp01ue1.configuration-anomaly-detection-production%%22,%%22accountId%%22:%%22%[2]s%%22%%7D%%5D,%%22expression%%22:%%22fields%%20message%%5Cn%%7C%%20filter%%20kubernetes.pod_name%%20like%%20%%5C%%22%s%%5C%%22%%22,%%22statsGroups%%22:%%5B%%5D%%7D%%5D,%%22range%%22:%%7B%%22from%%22:%%22now-1h%%22,%%22to%%22:%%22now%%22%%7D,%%22panelsState%%22:%%7B%%22logs%%22:%%7B%%22visualisationType%%22:%%22logs%%22%%7D%%7D%%7D%%7D&orgId=1", grafanaURL, awsAccountID, pipelineRunName) |
There was a problem hiding this comment.
This way you (should) only have to add the awsAccountId a single time instead of 4 times.
3 participants
…command
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests