Skip to content

Conversation

@arturshadnik
Copy link
Member

@arturshadnik arturshadnik commented Nov 3, 2025

Summary by CodeRabbit

  • Chores
    • Updated webhook certificate and cluster issuer deployment configurations to streamline Helm resource management lifecycle.
    • Modified resource deployment handling to ensure consistent configuration management across deployment environments.

@coderabbitai
Copy link

coderabbitai bot commented Nov 3, 2025

Walkthrough

The pull request modifies Helm hook annotations across admission webhook and cluster issuer configurations. Two chart template resources transition away from static pre-install/pre-upgrade hooks (one completely removed, one replaced with dynamic template injection), and a development configuration removes its hook annotation.

Changes

Cohort / File(s) Change Summary
Chart Templates - Helm Hook Annotation Updates
fleetconfig-controller/charts/fleetconfig-controller/templates/admission-webhooks/serving-cert.yaml, fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml
Removed static helm.sh/hook: pre-install,pre-upgrade from serving-cert Certificate. Replaced static hook annotation in clusterissuer with dynamic chart-wide annotations via include "chart.annotations".
Development Configuration - Hook Removal
fleetconfig-controller/hack/dev/cluster-issuer.yaml
Removed helm.sh/hook: pre-install,pre-upgrade annotation from ClusterIssuer metadata.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the rationale for divergent approaches (complete removal vs. dynamic template replacement across files)
  • Confirm include "chart.annotations" template exists and functions correctly in clusterissuer.yaml
  • Validate that removing pre-install/pre-upgrade hooks does not disrupt certificate/issuer initialization timing in deployment workflows

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing pre-install hooks from cert-manager related assets across multiple files.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a6b610 and 32c8b62.

📒 Files selected for processing (3)
  • fleetconfig-controller/charts/fleetconfig-controller/templates/admission-webhooks/serving-cert.yaml (0 hunks)
  • fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml (1 hunks)
  • fleetconfig-controller/hack/dev/cluster-issuer.yaml (0 hunks)
💤 Files with no reviewable changes (2)
  • fleetconfig-controller/charts/fleetconfig-controller/templates/admission-webhooks/serving-cert.yaml
  • fleetconfig-controller/hack/dev/cluster-issuer.yaml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-22T17:55:52.159Z
Learnt from: TylerGillson
Repo: open-cluster-management-io/lab PR: 51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.159Z
Learning: In the open-cluster-management-io/lab repository, chart versioning for fleetconfig-controller is handled automatically via GitHub release workflows, not through manual version bumps in Chart.yaml during regular PRs.

Applied to files:

  • fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml
📚 Learning: 2025-08-27T21:58:32.141Z
Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 58
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-27T21:58:32.141Z
Learning: In the open-cluster-management-io/lab repository, the fleetconfig-controller follows a workflow where chart version bumps (in README.md and values.yaml) are included in PRs before the corresponding Docker image exists. The Docker image is built and pushed automatically via GitHub release workflows after the PR is merged and tagged, making the referenced version available.

Applied to files:

  • fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml
📚 Learning: 2025-09-22T19:26:11.020Z
Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 59
File: fleetconfig-controller/test/data/fleetconfig-v1alpha1.yaml:47-53
Timestamp: 2025-09-22T19:26:11.020Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller tests, the kubeconfigKey is intentionally set to "value" in test fixtures (fleetconfig-v1alpha1.yaml, fleetconfig-values.yaml) because that's how the test harness provisions the kubeconfig secret during test setup. This differs from the chart default of "kubeconfig" but is correct for the test environment.

Applied to files:

  • fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml
📚 Learning: 2025-09-25T23:31:11.630Z
Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/charts/fleetconfig-controller/templates/ocm/fcc-addon/addon-template.yaml:110-112
Timestamp: 2025-09-25T23:31:11.630Z
Learning: The fleetconfig-controller-manager spoke agent requires create/update/patch/delete permissions on CustomResourceDefinitions because `clusteradm upgrade klusterlet` operations need create/update permissions and cleanup operations require delete permissions for proper lifecycle management.

Applied to files:

  • fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml
📚 Learning: 2025-09-22T18:42:03.404Z
Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 59
File: fleetconfig-controller/PROJECT:28-31
Timestamp: 2025-09-22T18:42:03.404Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller, the PROJECT file defines multiple API resources with different webhook configurations: FleetConfig v1alpha1 has defaulting: true (requiring MutatingWebhookConfiguration), while Hub and Spoke v1beta1 resources have defaulting: false. MutatingWebhookConfiguration resources in the manifests serve the v1alpha1 FleetConfig, not the v1beta1 Hub/Spoke resources.

Applied to files:

  • fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: e2e (fleetconfig-controller) / e2e
  • GitHub Check: e2e (fleetconfig-controller) / e2e
  • GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
  • GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
  • GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
🔇 Additional comments (1)
fleetconfig-controller/charts/fleetconfig-controller/templates/clusterissuer.yaml (1)

8-8: No issues found. The chart.annotations helper is properly defined and the lifecycle change is intentional.

Verification confirms that the chart.annotations helper in _helpers.tpl correctly outputs Helm release metadata, and the nindent 4 indentation is correct for the YAML structure. The shift from pre-install timing to regular deployment flow is consistent across related resources (e.g., serving-cert.yaml also uses chart.annotations). Since cert-manager resources are idempotent and dependencies are handled through object references, this timing change is safe and appears to be part of an intentional refactoring to remove pre-install hooks.


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.

❤️ Share

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

@openshift-ci
Copy link

openshift-ci bot commented Nov 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arturshadnik, TylerGillson

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

The pull request process is described here

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-merge-bot openshift-merge-bot bot merged commit 704faa7 into open-cluster-management-io:main Nov 3, 2025
18 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants