Feat: Demo changes for FullSend Skill Testing - #370
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Sushant2504 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 |
|
/test ci/prow/coverage |
|
/test all |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:18 AM UTC · Completed 6:32 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.08 |
|
Risk Assessment: moderate (2/5) DetailsLow-risk change: a single new test file (119 lines) with no protected paths, no security-sensitive files, no dependency or CI changes, and a 100% test file ratio; moderate baseline for new file with no git history. |
ReviewFindingsCritical
High
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| It("should accept lowercase alphanumeric names", func() { | ||
| err := ValidateNamespace("my-namespace") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
[critical] logic-error
The test file calls ValidateNamespace() and IsSystemNamespace() but neither function exists in the namespace package. The only function in pkg/util/namespace/namespace.go is GetOperatorNamespace(). This package will fail to compile. The PR body claims all tests pass and go build succeeds, but these claims are false.
Suggested fix: Either add the implementations of ValidateNamespace and IsSystemNamespace to pkg/util/namespace/namespace.go in this PR, or remove these tests until the functions are implemented.
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestNamespace(t *testing.T) { |
There was a problem hiding this comment.
[medium] code-organization
The codebase convention places the Ginkgo test bootstrap function in a dedicated *_suite_test.go file. All 5 existing test packages follow this pattern. This file inlines the bootstrap into the spec file.
Suggested fix: Extract the TestNamespace function (lines 10-13) into a new file namespace_suite_test.go.
|
|
||
| func TestNamespace(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, "Namespace Suite") |
There was a problem hiding this comment.
[low] naming-convention
Suite description 'Namespace Suite' follows the existing ' Suite' pattern acceptably.
| import ( | ||
| "testing" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" |
There was a problem hiding this comment.
[low] pattern-inconsistency
Uses ginkgo/v2 while existing test files use ginkgo v1. CLAUDE.md recommends v2 for new code, so this aligns with the stated migration direction.
The ocm-agent-operator creates a ServiceMonitor for Prometheus to scrape port 8383 metrics, but the existing NetworkPolicies only allow Alertmanager and MUO ingress. This causes TargetDown alerts because prometheus-k8s pods from openshift-monitoring cannot reach the ocm-agent metrics endpoint. Add a new NetworkPolicy that allows prometheus-k8s pods (selected by app.kubernetes.io/name=prometheus) from openshift-monitoring to reach the ocm-agent. The policy is created in both fleet and non-fleet modes since Prometheus scraping is needed in both. Uses the existing dispatch-key pattern (like NamespaceRHOBS) so NamespacePrometheus maps back to the real openshift-monitoring namespace via callerNamespace(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rset ROSAENG-62697: align ManagedNotification severity with OCM Service Log's new HCC-aligned labels
|
/fs-review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #370 +/- ##
==========================================
+ Coverage 65.39% 66.07% +0.68%
==========================================
Files 23 23
Lines 1598 1636 +38
==========================================
+ Hits 1045 1081 +36
- Misses 473 475 +2
Partials 80 80
🚀 New features to boost your workflow:
|
|
/fs-review |
1 similar comment
|
/fs-review |
|
@Sushant2504: The following tests 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. |
|
/fs-review |
|
🤖 Review · Started 11:08 PM UTC Commit: |
5 participants
Summary
This PR adds utility functions for Kubernetes namespace validation and classification, along with enhanced logging in the OCMAgent controller for better observability. These changes demonstrate typical code quality improvements and feature additions for PR review tool testing.
Changes
New Utilities (pkg/util/namespace/)
Test Coverage
Controller Improvements (controllers/ocmagent/)
Test Results
✅ All 17 tests passing
✅ go build successful
✅ go mod tidy clean
Type of Change
Checklist