OCPBUGS-84307: Clarify --base-domain flag default behavior#8381
OCPBUGS-84307: Clarify --base-domain flag default behavior#8381dhgautam99 wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-low
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-84307, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 |
openshift-ci
Bot
added
do-not-merge/needs-area
area/cli
openshift-ci
Bot
added
area/control-plane-operator
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-84307, which is valid. 3 validation(s) were run on this bug
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. |
dhgautam99
force-pushed
the
add-context-base-domain
branch
from
50c4689 to
d9dabe1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8381 +/- ##
==========================================
+ Coverage 39.72% 40.07% +0.35%
==========================================
Files 751 751
Lines 92461 92838 +377
==========================================
+ Hits 36729 37205 +476
+ Misses 53076 52946 -130
- Partials 2656 2687 +31
... and 45 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/cluster/core/create_test.go (1)
49-59: 💤 Low valueConsider clarifying the subtest name to reflect pflag-level vs. runtime behavior.
The subtest is named "When base-domain is not provided it should default to empty", but the updated flag description documents that omitting
--base-domain"defaults to management cluster's apps domain" at runtime. The test correctly asserts the pflag-level default ("") — which is accurate — but the name "default to empty" could mislead a future reader into thinking the system's end-to-end behavior is an empty string rather than the resolved apps domain.✏️ Suggested rename
- t.Run("When base-domain is not provided it should default to empty", func(t *testing.T) { + t.Run("When base-domain is not provided the flag value should be empty (resolved at runtime to management cluster apps domain)", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/core/create_test.go` around lines 49 - 59, Rename the subtest to clarify it asserts the flag's pflag-level default rather than runtime resolution: update the t.Run description string (the subtest around NewWithT, DefaultOptions, pflag.NewFlagSet, BindOptions and opts.BaseDomain) to something like "When --base-domain flag is omitted, pflag default should be empty" so readers understand the test checks the parsed flag default (opts.BaseDomain == "") not the runtime resolved apps domain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/cluster/core/create_test.go`:
- Around line 49-59: Rename the subtest to clarify it asserts the flag's
pflag-level default rather than runtime resolution: update the t.Run description
string (the subtest around NewWithT, DefaultOptions, pflag.NewFlagSet,
BindOptions and opts.BaseDomain) to something like "When --base-domain flag is
omitted, pflag default should be empty" so readers understand the test checks
the parsed flag default (opts.BaseDomain == "") not the runtime resolved apps
domain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fc0fc7ab-2237-4e16-981f-9063a3689d9f
📒 Files selected for processing (1)
cmd/cluster/core/create_test.go
dhgautam99
force-pushed
the
add-context-base-domain
branch
from
4bfc2c1 to
14b7ac5
Compare
| flags.StringVar(&opts.Namespace, "namespace", opts.Namespace, "A namespace to contain the generated resources") | ||
| flags.StringVar(&opts.Name, "name", opts.Name, "A name for the cluster") | ||
| flags.StringVar(&opts.BaseDomain, "base-domain", opts.BaseDomain, "The ingress base domain for the cluster") | ||
| flags.StringVar(&opts.BaseDomain, "base-domain", opts.BaseDomain, "The ingress base domain for the cluster. When omitted, defaults to management cluster's apps domain.") |
There was a problem hiding this comment.
This is only true for kubevirt, so not generally applicable to all platforms.
There was a problem hiding this comment.
Reflected the default behaviour for kubevirt specific HCP cluster.
dhgautam99
force-pushed
the
add-context-base-domain
branch
from
14b7ac5 to
e7ff1e5
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/test images |
Update the help text for the --base-domain flag in the create command to indicate that when omitted, it defaults to the management cluster's apps domain. Refs: OCPBUGS-84307 Signed-off-by: Dhruv Gautam <dgautam@redhat.com>
dhgautam99
force-pushed
the
add-context-base-domain
branch
from
e7ff1e5 to
a02d929
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
dhgautam99
force-pushed
the
add-context-base-domain
branch
from
a02d929 to
e96ecb9
Compare
|
Now I have the complete picture. Here is the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is a race condition in Codecov evaluation, not a real coverage regression. The repository's Root CauseThe Configuration issue: The repo's Timeline of events (all times UTC, May 8 2026):
Result of premature evaluation: The base branch ( Flags at evaluation time:
Recommendations
Evidence
|
Verify that BindOptions correctly wires CLI flags to the RawCreateOptions struct, covering the --base-domain flag help text change. Refs: OCPBUGS-84307 Signed-off-by: Dhruv Gautam <dgautam@redhat.com>
dhgautam99
force-pushed
the
add-context-base-domain
branch
from
934d055 to
83edfdd
Compare
|
/approve |
openshift-ci
Bot
added
the
approved
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, csrwng, dhgautam99 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 |
|
/jira backport release-4.22 |
|
@dhgautam99: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: 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-robot: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/test e2e-aks |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
@dhgautam99: 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. |
6 participants
What this PR does / why we need it:
Updates the help text for the
--base-domainflag in thehypershift create clustercommandto clarify that when omitted, it defaults to the management cluster's apps domain. This
improves discoverability so users know the flag is optional and understand the fallback behavior.
Which issue(s) this PR fixes:
Fixes OCPBUGS-84307
Special notes for your reviewer:
Single-line help text change, no functional impact.
Checklist:
Summary by CodeRabbit
Documentation
Tests