Skip to content

USHIFT-6798: C2CC: RemoteClusters.microshift.io/v1alpha1 Custom Resource#6727

Open
pmtk wants to merge 3 commits into
openshift:mainfrom
pmtk:c2cc/remote-clusters-crd
Open

USHIFT-6798: C2CC: RemoteClusters.microshift.io/v1alpha1 Custom Resource#6727
pmtk wants to merge 3 commits into
openshift:mainfrom
pmtk:c2cc/remote-clusters-crd

Conversation

@pmtk
Copy link
Copy Markdown
Member

@pmtk pmtk commented May 25, 2026

Summary by CodeRabbit

  • New Features

    • Added a RemoteCluster CRD and a controller to manage per-remote-cluster healthcheck resources.
    • Introduced clusterToCluster.dns.probeInterval setting (default 10s, range 1s–5m).
  • Tests

    • Added unit and integration tests covering probe interval validation, RemoteCluster reconciliation, and end-to-end CR lifecycle.
  • Documentation

    • Updated configuration docs and default config examples to include the new probeInterval setting.
  • Chores

    • Added tooling and a make target to generate CRDs and client code.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 25, 2026
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented May 25, 2026

@pmtk: This pull request references USHIFT-6798 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 "5.0.0" version, but no target version was set.

Details

In 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 openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 25, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a RemoteCluster CRD, generation tooling, C2CC ProbeInterval config with validation and resolved ProbeIP, a healthcheck manager that reconciles RemoteCluster CRs, asset helpers for CRD deletion, unit tests, and Robot e2e tests.

Changes

C2CC RemoteCluster Healthcheck

Layer / File(s) Summary
RemoteCluster API Type Definition
pkg/apis/microshift/v1alpha1/doc.go, pkg/apis/microshift/v1alpha1/groupversion_info.go, pkg/apis/microshift/v1alpha1/types.go
Introduces RemoteCluster API surface, group/version registration, and kubebuilder markers.
CRD Manifest
assets/crd/microshift.io_remoteclusters.yaml, scripts/auto-rebase/assets.yaml
Adds generated cluster-scoped RemoteCluster CRD with OpenAPI v3 schema and status subresource; registers CRD as generated in assets registry.
CRD Asset Helpers
pkg/assets/crd.go
Adds CRD decoding helper and exported DeleteCRDs that deletes CRDs by name using kubeconfig-based apiextensions client, suppressing NotFound and aggregating other errors.
Configuration, Validation & Docs
pkg/config/c2cc.go, pkg/config/c2cc_test.go, pkg/config/config.go, cmd/generate-config/config/config-openapi-spec.json, docs/user/howto_config.md, packaging/microshift/config.yaml
Adds clusterToCluster.dns.probeInterval (default "10s"), parses/validates it (1s–5m), computes deterministic ProbeIP from service CIDR, updates defaults and merging, updates JSON OpenAPI schema and docs, and adds unit tests for parsing, defaults, and ProbeIP resolution.
Controller Integration & Lifecycle
pkg/controllers/c2cc/controller.go
Wires healthcheck subsystem: applies CRD and waits for establishment on startup, creates MicroShift typed client, initializes healthcheck manager, adds healthcheck-crs reconcile step, and deletes CRD on cleanup.
Healthcheck Reconciliation Logic
pkg/controllers/c2cc/healthcheck.go
Implements healthcheckCRManager to build desired RemoteCluster CRs (probeTarget ProbeIP:8080, probeInterval from resolved config), create/update/delete managed CRs, and derive deterministic CR names from next-hop IPs.
Healthcheck Unit Tests
pkg/controllers/c2cc/healthcheck_test.go
Unit tests for CR naming, desired CR construction, and reconcile behavior: create missing CRs, delete stale managed CRs, and update CRs when probe interval changes.
CRD Generation Tooling
Makefile, scripts/generate-crds.sh
Adds generate-crds make target and script to run controller-gen and OpenShift codegen to emit CRD YAML and typed clients.
End-to-End Integration Tests
test/suites/c2cc/healthcheck.robot
Robot Framework suite validating RemoteCluster CRD presence, per-cluster CR count, computed probeTarget/probeInterval formatting, and managed-by label across clusters.

Sequence Diagram

sequenceDiagram
  participant ConfigMgr as Config Manager
  participant C2CCCtrl as C2CC Controller
  participant KubeAPI as Kubernetes API
  participant HealthMgr as Healthcheck Manager

  ConfigMgr->>ConfigMgr: Validate ProbeInterval (1s–5m)
  ConfigMgr->>ConfigMgr: Compute ProbeIP from service CIDR (11th host)
  ConfigMgr->>C2CCCtrl: Provide resolved C2CC config

  C2CCCtrl->>KubeAPI: Apply RemoteCluster CRD
  C2CCCtrl->>KubeAPI: Wait for CRD establishment
  C2CCCtrl->>HealthMgr: Create healthcheck manager with typed client

  HealthMgr->>KubeAPI: List existing RemoteCluster CRs (label filter)
  HealthMgr->>KubeAPI: Create missing RemoteCluster CRs
  HealthMgr->>KubeAPI: Update CRs when ProbeTarget/ProbeInterval differ
  HealthMgr->>KubeAPI: Delete stale managed CRs

  Note over C2CCCtrl: On shutdown
  C2CCCtrl->>KubeAPI: Delete RemoteCluster CRD
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • openshift/microshift#6638: Modifies C2CC config validation and adds related resolved fields in the same configuration layer.

Suggested reviewers

  • kasturinarra
  • eslutsky
🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: introducing a RemoteClusters Custom Resource Definition under the microshift.io API group.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PR contains only standard Go testing.T tests and Robot Framework tests with static, deterministic names (no Ginkgo tests). All test names are hardcoded literal strings with no dynamic content.
Test Structure And Quality ✅ Passed PR contains no Ginkgo tests—only traditional Go unit tests (using testing package) and Robot Framework tests. Check is not applicable.
Microshift Test Compatibility ✅ Passed No Ginkgo e2e tests were added in this PR. The PR adds Go unit tests (using testing.T) and a Robot Framework test suite, neither of which are subject to the Ginkgo test compatibility check.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No Ginkgo e2e tests found. PR adds Go unit tests and Robot Framework integration test, neither requiring SNO compatibility checks.
Topology-Aware Scheduling Compatibility ✅ Passed PR adds RemoteCluster CRD and healthcheck controller without introducing deployment manifests, pod affinity, node selectors, or topology-unaware scheduling constraints.
Ote Binary Stdout Contract ✅ Passed PR introduces no OTE test binaries—only API types, controller code, standard Go unit tests, Robot Framework tests, and build scripts. The OTE stdout contract check is not applicable.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No Ginkgo e2e tests added. PR contains only Go unit tests (testing.T) and Robot Framework tests, which fall outside the scope of this check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 25, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmtk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/generate-config/config/config-openapi-spec.json`:
- Around line 206-209: Update the probeInterval property in
config-openapi-spec.json to include an OpenAPI-level default and pattern
validation: add "default": "10s" and add a "pattern" such as
"^[0-9]+(ns|us|µs|ms|s|m|h)$" so consumers validate Go-style duration tokens;
keep the existing description about 1s–5m bounds (and ensure runtime/server-side
enforcement still checks the numeric range if regex cannot express the min/max).
Ensure you modify the probeInterval object (the JSON key "probeInterval") to
include these new fields.

In `@pkg/controllers/c2cc/healthcheck.go`:
- Around line 37-54: Reconcile should not hard-fail on Create errors caused by
an existing same-name CR missing our managed-by label; in the Create error
branch (around h.client.RemoteClusters().Create called for entries in desired)
detect apierrors.IsAlreadyExists(err) and instead fetch the existing object
(h.client.RemoteClusters().Get(ctx, name, metav1.GetOptions{})), add or set the
managedByLabel to managerName on its ObjectMeta.Labels, then Update or Patch
that existing RemoteCluster (handling update errors) so the resource becomes
managed; for non-AlreadyExists errors keep returning the error. Ensure you
import and use k8s.io/apimachinery/pkg/api/errors (apierrors) and reference the
identifiers h.client.RemoteClusters().Create, h.client.RemoteClusters().Get,
h.client.RemoteClusters().Update, desired, managedByLabel, and managerName.

In `@test/suites/c2cc/07-healthcheck.robot`:
- Line 85: The assertion uses a literal "${expected_ip}:8080" which fails for
IPv6 — build the expected host:port using bracketed IPv6 syntax when the IP
contains a colon; e.g. compute an expected_hostport variable by checking
${expected_ip} for ":" and setting expected_hostport to "[${expected_ip}]:8080"
for IPv6 or "${expected_ip}:8080" for IPv4, then compare ${stdout} to that
expected_hostport with Should Be Equal As Strings.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 5bab752a-3c53-473e-928e-f6cd3a4a4b16

📥 Commits

Reviewing files that changed from the base of the PR and between ca5e605 and 0f20caf.

⛔ Files ignored due to path filters (22)
  • pkg/apis/microshift/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • pkg/generated/clientset/versioned/clientset.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/fake/clientset_generated.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/fake/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/fake/register.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/scheme/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/scheme/register.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_microshift_client.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_remotecluster.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/generated_expansion.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/microshift_client.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/remotecluster.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/factory.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/generic.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/internalinterfaces/factory_interfaces.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/microshift/interface.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/microshift/v1alpha1/interface.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/microshift/v1alpha1/remotecluster.go is excluded by !**/generated/**
  • pkg/generated/listers/microshift/v1alpha1/expansion_generated.go is excluded by !**/generated/**
  • pkg/generated/listers/microshift/v1alpha1/remotecluster.go is excluded by !**/generated/**
📒 Files selected for processing (19)
  • Makefile
  • assets/crd/microshift.io_remoteclusters.yaml
  • cmd/generate-config/config/config-openapi-spec.json
  • docs/user/howto_config.md
  • packaging/microshift/config.yaml
  • pkg/apis/microshift/v1alpha1/doc.go
  • pkg/apis/microshift/v1alpha1/groupversion_info.go
  • pkg/apis/microshift/v1alpha1/types.go
  • pkg/assets/crd.go
  • pkg/config/c2cc.go
  • pkg/config/c2cc_test.go
  • pkg/config/config.go
  • pkg/controllers/c2cc/controller.go
  • pkg/controllers/c2cc/healthcheck.go
  • pkg/controllers/c2cc/healthcheck_test.go
  • scripts/auto-rebase/assets.yaml
  • scripts/boilerplate.go.txt
  • scripts/generate-crds.sh
  • test/suites/c2cc/07-healthcheck.robot

Comment thread cmd/generate-config/config/config-openapi-spec.json
Comment thread pkg/controllers/c2cc/healthcheck.go
Comment thread test/suites/c2cc/07-healthcheck.robot Outdated
@pmtk pmtk force-pushed the c2cc/remote-clusters-crd branch from 0f20caf to fe24b49 Compare May 25, 2026 09:31
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/suites/c2cc/07-healthcheck.robot`:
- Line 95: Update the regexp used in the healthcheck test so it matches the
entire probe interval string; replace the pattern in the assertion that checks
${interval} (the line using Should Match Regexp) from ^[0-9]+(s|m|h) to an
anchored pattern ^[0-9]+(s|m|h)$ so values like "10sfoo" no longer pass.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: fb156625-8c3b-42df-8034-069ed4c299f7

📥 Commits

Reviewing files that changed from the base of the PR and between 0f20caf and fe24b49.

📒 Files selected for processing (6)
  • pkg/assets/crd.go
  • pkg/config/c2cc_test.go
  • pkg/controllers/c2cc/controller.go
  • pkg/controllers/c2cc/healthcheck.go
  • pkg/controllers/c2cc/healthcheck_test.go
  • test/suites/c2cc/07-healthcheck.robot

Comment thread test/suites/c2cc/07-healthcheck.robot Outdated
@pmtk pmtk force-pushed the c2cc/remote-clusters-crd branch from fe24b49 to 638d177 Compare May 25, 2026 10:22
@pmtk pmtk marked this pull request as ready for review May 25, 2026 10:29
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2026
@openshift-ci openshift-ci Bot requested review from jogeo and kasturinarra May 25, 2026 10:30
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/config/c2cc_test.go`:
- Around line 863-896: The tests call cfg.fillDefaults() but ignore its returned
error, which can mask setup failures; update each test setup (in TestC2CC_*
blocks) to capture the error from Config.fillDefaults() and assert it succeeded
before proceeding (e.g., call err := cfg.fillDefaults() and assert/require no
error), so subsequent calls to cfg.incorporateUserSettings and assertions on
cfg.C2CC.ProbeInterval run only if defaults initialization succeeded.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: cec90440-4564-4dee-8100-9fd0b40cbb71

📥 Commits

Reviewing files that changed from the base of the PR and between fe24b49 and 638d177.

📒 Files selected for processing (10)
  • docs/user/howto_config.md
  • packaging/microshift/config.yaml
  • pkg/assets/crd.go
  • pkg/config/c2cc.go
  • pkg/config/c2cc_test.go
  • pkg/config/config.go
  • pkg/controllers/c2cc/controller.go
  • pkg/controllers/c2cc/healthcheck.go
  • pkg/controllers/c2cc/healthcheck_test.go
  • test/suites/c2cc/07-healthcheck.robot
✅ Files skipped from review due to trivial changes (1)
  • docs/user/howto_config.md

Comment thread pkg/config/c2cc_test.go
Comment on lines +863 to +896
cfg := &Config{}
cfg.fillDefaults()
assert.Equal(t, "10s", cfg.C2CC.ProbeInterval)
}

func TestC2CC_IncorporateUserSettings(t *testing.T) {
t.Run("user overrides probe interval", func(t *testing.T) {
cfg := &Config{}
cfg.fillDefaults()

user := &Config{
C2CC: C2CC{
ProbeInterval: "30s",
},
}
cfg.incorporateUserSettings(user)
assert.Equal(t, "30s", cfg.C2CC.ProbeInterval)
})

t.Run("user sets remoteClusters without probeInterval preserves default", func(t *testing.T) {
cfg := &Config{}
cfg.fillDefaults()

user := &Config{
C2CC: C2CC{
RemoteClusters: []RemoteCluster{{
NextHop: "10.100.0.2",
ClusterNetwork: []string{"10.45.0.0/16"},
ServiceNetwork: []string{"10.46.0.0/16"},
}},
},
}
cfg.incorporateUserSettings(user)
assert.Equal(t, "10s", cfg.C2CC.ProbeInterval)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert fillDefaults() success in test setup.

Line 864, Line 871, and Line 884 ignore the returned error. If default initialization fails, later assertions can fail with misleading signals.

Suggested fix
 func TestC2CC_ProbeIntervalDefault(t *testing.T) {
 	cfg := &Config{}
-	cfg.fillDefaults()
+	require.NoError(t, cfg.fillDefaults())
 	assert.Equal(t, "10s", cfg.C2CC.ProbeInterval)
 }

 func TestC2CC_IncorporateUserSettings(t *testing.T) {
 	t.Run("user overrides probe interval", func(t *testing.T) {
 		cfg := &Config{}
-		cfg.fillDefaults()
+		require.NoError(t, cfg.fillDefaults())

 		user := &Config{
 			C2CC: C2CC{
 				ProbeInterval: "30s",
@@
 	t.Run("user sets remoteClusters without probeInterval preserves default", func(t *testing.T) {
 		cfg := &Config{}
-		cfg.fillDefaults()
+		require.NoError(t, cfg.fillDefaults())

 		user := &Config{
 			C2CC: C2CC{
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/config/c2cc_test.go` around lines 863 - 896, The tests call
cfg.fillDefaults() but ignore its returned error, which can mask setup failures;
update each test setup (in TestC2CC_* blocks) to capture the error from
Config.fillDefaults() and assert it succeeded before proceeding (e.g., call err
:= cfg.fillDefaults() and assert/require no error), so subsequent calls to
cfg.incorporateUserSettings and assertions on cfg.C2CC.ProbeInterval run only if
defaults initialization succeeded.

@pmtk pmtk force-pushed the c2cc/remote-clusters-crd branch from 638d177 to 9da5a21 Compare May 25, 2026 11:25
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/suites/c2cc/healthcheck.robot (1)

91-95: 💤 Low value

Duration regex may be too narrow.

The pattern ^[0-9]+(s|m|h)$ matches simple durations like "30s" but won't match compound Go durations like "1m30s" or millisecond units like "500ms". If the controller can produce such formats, this test could spuriously fail.

Consider ^[0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h)+$ or simply assert non-empty if strict format isn't required.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/suites/c2cc/healthcheck.robot` around lines 91 - 95, The duration regex
in the healthcheck test around the ${interval} value is too narrow and rejects
compound or millisecond Go durations; update the assertion in the test that
follows the Oc On Cluster call (the variable ${interval} populated by the oc get
remoteclusters... jsonpath) to use a more permissive regex such as one that
accepts optional decimals and units (e.g. allow ns|us|µs|ms|s|m|h and compound
forms) or simply assert non-empty; replace the current Should Match Regexp line
with the broader pattern (or change to only Should Not Be Empty) to avoid
spurious failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/suites/c2cc/healthcheck.robot`:
- Around line 91-95: The duration regex in the healthcheck test around the
${interval} value is too narrow and rejects compound or millisecond Go
durations; update the assertion in the test that follows the Oc On Cluster call
(the variable ${interval} populated by the oc get remoteclusters... jsonpath) to
use a more permissive regex such as one that accepts optional decimals and units
(e.g. allow ns|us|µs|ms|s|m|h and compound forms) or simply assert non-empty;
replace the current Should Match Regexp line with the broader pattern (or change
to only Should Not Be Empty) to avoid spurious failures.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 0646dc20-4db3-4836-b3e6-582726ddbdc2

📥 Commits

Reviewing files that changed from the base of the PR and between 638d177 and 9da5a21.

📒 Files selected for processing (6)
  • pkg/assets/crd.go
  • pkg/config/c2cc_test.go
  • pkg/controllers/c2cc/controller.go
  • pkg/controllers/c2cc/healthcheck.go
  • pkg/controllers/c2cc/healthcheck_test.go
  • test/suites/c2cc/healthcheck.robot

@pmtk pmtk force-pushed the c2cc/remote-clusters-crd branch from 9da5a21 to 32ebff5 Compare May 25, 2026 11:36
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
pkg/config/c2cc_test.go (1)

863-864: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert fillDefaults() success in test setup.

Line 864, Line 871, and Line 884 ignore fillDefaults() errors; this can hide setup failures and mislead assertions.

Suggested fix
 func TestC2CC_ProbeIntervalDefault(t *testing.T) {
 	cfg := &Config{}
-	cfg.fillDefaults()
+	require.NoError(t, cfg.fillDefaults())
 	assert.Equal(t, "10s", cfg.C2CC.ProbeInterval)
 }

 func TestC2CC_IncorporateUserSettings(t *testing.T) {
 	t.Run("user overrides probe interval", func(t *testing.T) {
 		cfg := &Config{}
-		cfg.fillDefaults()
+		require.NoError(t, cfg.fillDefaults())

 		user := &Config{
 			C2CC: C2CC{
 				ProbeInterval: "30s",
@@
 	t.Run("user sets remoteClusters without probeInterval preserves default", func(t *testing.T) {
 		cfg := &Config{}
-		cfg.fillDefaults()
+		require.NoError(t, cfg.fillDefaults())

 		user := &Config{
 			C2CC: C2CC{

Also applies to: 870-871, 883-884

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/config/c2cc_test.go` around lines 863 - 864, The test setup calls
cfg.fillDefaults() but ignores its returned error, which can hide failures;
update the test in pkg/config/c2cc_test.go to assert that fillDefaults() returns
nil (e.g., fail the test or use t.Fatal/t.Fatalf) after calling
cfg.fillDefaults() for the occurrences around the Config variable (refs: Config
and fillDefaults()) so any setup error is surfaced immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/suites/c2cc/healthcheck.robot`:
- Line 95: The current regex for ${interval} only accepts single integer units
(e.g., 10s) but probeInterval can be any Go duration (fractional and compound
like 1.5s or 1m30s); update the Robot assertion replacing ^[0-9]+(s|m|h)$ with a
pattern that accepts repeated numeric (with optional fractional) + Go unit
groups, e.g. ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ so Should Match Regexp
${interval} validates durations parsed by time.ParseDuration.

---

Duplicate comments:
In `@pkg/config/c2cc_test.go`:
- Around line 863-864: The test setup calls cfg.fillDefaults() but ignores its
returned error, which can hide failures; update the test in
pkg/config/c2cc_test.go to assert that fillDefaults() returns nil (e.g., fail
the test or use t.Fatal/t.Fatalf) after calling cfg.fillDefaults() for the
occurrences around the Config variable (refs: Config and fillDefaults()) so any
setup error is surfaced immediately.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: b7960763-96b2-4834-a9de-9133cf3b3d07

📥 Commits

Reviewing files that changed from the base of the PR and between 9da5a21 and 32ebff5.

⛔ Files ignored due to path filters (22)
  • pkg/apis/microshift/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • pkg/generated/clientset/versioned/clientset.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/fake/clientset_generated.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/fake/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/fake/register.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/scheme/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/scheme/register.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/doc.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_microshift_client.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_remotecluster.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/generated_expansion.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/microshift_client.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/microshift/v1alpha1/remotecluster.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/factory.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/generic.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/internalinterfaces/factory_interfaces.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/microshift/interface.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/microshift/v1alpha1/interface.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/microshift/v1alpha1/remotecluster.go is excluded by !**/generated/**
  • pkg/generated/listers/microshift/v1alpha1/expansion_generated.go is excluded by !**/generated/**
  • pkg/generated/listers/microshift/v1alpha1/remotecluster.go is excluded by !**/generated/**
📒 Files selected for processing (19)
  • Makefile
  • assets/crd/microshift.io_remoteclusters.yaml
  • cmd/generate-config/config/config-openapi-spec.json
  • docs/user/howto_config.md
  • packaging/microshift/config.yaml
  • pkg/apis/microshift/v1alpha1/doc.go
  • pkg/apis/microshift/v1alpha1/groupversion_info.go
  • pkg/apis/microshift/v1alpha1/types.go
  • pkg/assets/crd.go
  • pkg/config/c2cc.go
  • pkg/config/c2cc_test.go
  • pkg/config/config.go
  • pkg/controllers/c2cc/controller.go
  • pkg/controllers/c2cc/healthcheck.go
  • pkg/controllers/c2cc/healthcheck_test.go
  • scripts/auto-rebase/assets.yaml
  • scripts/boilerplate.go.txt
  • scripts/generate-crds.sh
  • test/suites/c2cc/healthcheck.robot
✅ Files skipped from review due to trivial changes (2)
  • pkg/apis/microshift/v1alpha1/doc.go
  • docs/user/howto_config.md

Comment thread test/suites/c2cc/healthcheck.robot
@pmtk pmtk force-pushed the c2cc/remote-clusters-crd branch from 32ebff5 to 467958f Compare May 25, 2026 12:41
@pmtk pmtk force-pushed the c2cc/remote-clusters-crd branch from 467958f to f6e1153 Compare May 25, 2026 13:57
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 25, 2026

@pmtk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/verify 467958f link true /test verify
ci/prow/test-rebase 467958f link false /test test-rebase

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants