MGMT-22518: Adding RHOAI application endpoint to the list of known hosts for SNO#9936
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pastequo: This pull request references MGMT-22518 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. |
WalkthroughAdds RHODS dashboard flag support to DNSMasq single-node manifest generation by checking the OpenShift AI operator status. The implementation conditionally includes RHODS dashboard DNS entries in the manifest template based on operator enablement state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
openshift-ci
Bot
added
the
size/M
openshift-ci
Bot
added
the
approved
|
@pastequo: This pull request references MGMT-22518 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.
🧹 Nitpick comments (2)
internal/network/manifests_generator.go (1)
114-114: Template conditional is correct; consider formatting it as a block for readability.
Keeping the conditional on separate lines reduces accidental template concatenation mistakes in future edits.Optional readability tweak
-{{if .RHODS_DASHBOARD_ENABLED}}address=/rhods-dashboard-redhat-ods-applications.apps.${CLUSTER_FULL_DOMAIN}/${HOST_IP}{{end}} +{{if .RHODS_DASHBOARD_ENABLED}} +address=/rhods-dashboard-redhat-ods-applications.apps.${CLUSTER_FULL_DOMAIN}/${HOST_IP} +{{end}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/manifests_generator.go` at line 114, The single-line template conditional risks accidental concatenation; refactor the DNS entry into a block by placing the opening tag {{if .RHODS_DASHBOARD_ENABLED}} on its own line, the address=... line on the next line using ${CLUSTER_FULL_DOMAIN} and ${HOST_IP}, and the closing {{end}} on its own line so the conditional and its content are clearly separated (references: .RHODS_DASHBOARD_ENABLED, ${CLUSTER_FULL_DOMAIN}, ${HOST_IP}).internal/network/manifests_generator_test.go (1)
540-553: Consider one extra negative case with a non-AI operator present.
Current negative coverage usesnil; adding a “different operator exists” case would guard against accidentallen(operators) > 0regressions in future refactors.Suggested test addition
+ It("does not include RHODS dashboard DNS when only non-AI operators are enabled", func() { + cluster := createCluster("", "3.3.3.0/24", + createInventory(createInterface("3.3.3.3/24"))) + cluster.Hosts[0].Bootstrap = true + cluster.Cluster.BaseDNSDomain = "test.com" + cluster.Cluster.Name = "test" + cluster.MonitoredOperators = []*models.MonitoredOperator{ + {Name: "local-storage-operator"}, + } + + created, err := createDnsmasqForSingleNode(logrus.New(), cluster) + Expect(err).To(Not(HaveOccurred())) + + dnsmasqScript := extractDnsmasqScript(created) + Expect(dnsmasqScript).NotTo(ContainSubstring("rhods-dashboard-redhat-ods-applications")) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/manifests_generator_test.go` around lines 540 - 553, Add a second negative test case to ensure presence of non-AI operators doesn't trigger RHODS DNS—duplicate the existing It block ("does not include RHODS dashboard DNS when OpenShift AI operator is not enabled") but set MonitoredOperators to a slice containing a non-AI operator (e.g., an operator with a different Name) instead of nil; call createDnsmasqForSingleNode and validate via extractDnsmasqScript that the output still does not ContainSubstring("rhods-dashboard-redhat-ods-applications"). This prevents regressions where code checks only for len(MonitoredOperators) > 0 rather than explicitly for the OpenShift AI operator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/network/manifests_generator_test.go`:
- Around line 540-553: Add a second negative test case to ensure presence of
non-AI operators doesn't trigger RHODS DNS—duplicate the existing It block
("does not include RHODS dashboard DNS when OpenShift AI operator is not
enabled") but set MonitoredOperators to a slice containing a non-AI operator
(e.g., an operator with a different Name) instead of nil; call
createDnsmasqForSingleNode and validate via extractDnsmasqScript that the output
still does not ContainSubstring("rhods-dashboard-redhat-ods-applications"). This
prevents regressions where code checks only for len(MonitoredOperators) > 0
rather than explicitly for the OpenShift AI operator.
In `@internal/network/manifests_generator.go`:
- Line 114: The single-line template conditional risks accidental concatenation;
refactor the DNS entry into a block by placing the opening tag {{if
.RHODS_DASHBOARD_ENABLED}} on its own line, the address=... line on the next
line using ${CLUSTER_FULL_DOMAIN} and ${HOST_IP}, and the closing {{end}} on its
own line so the conditional and its content are clearly separated (references:
.RHODS_DASHBOARD_ENABLED, ${CLUSTER_FULL_DOMAIN}, ${HOST_IP}).
ℹ️ 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 (2)
internal/network/manifests_generator.gointernal/network/manifests_generator_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gamli75, pastequo 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9936 +/- ##
==========================================
- Coverage 44.06% 43.10% -0.97%
==========================================
Files 414 414
Lines 72172 73832 +1660
==========================================
+ Hits 31803 31824 +21
- Misses 37494 39131 +1637
- Partials 2875 2877 +2
🚀 New features to boost your workflow:
|
|
/retest |
1 similar comment
|
/retest |
|
@pastequo: 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. |
3 participants
If the target cluster is a SNO cluster & openshift-ai operator is selected, then we add the rhods dashboard url to the dnsmasq config
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Deployed a SNO cluster and check by ssh-ing to the node that there is dns resolution
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
Release Notes