Skip to content

Conversation

Kousalya1998
Copy link
Contributor

No description provided.

Copy link

openshift-ci bot commented Sep 12, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Kousalya1998
Once this PR has been reviewed and has the lgtm label, please assign hugares for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Copy link
Contributor

Code Review by Gemini

### `argo-cd-apps/base/all-clusters/infra-deployments/kustomization.yaml`

*   **Issue**: The `disable-self-provisioning-for-all-cluster` resource is removed from the `all-clusters` base kustomization. This, in conjunction with the deletion of `configs/disable-self-provisioning-for-all-cluster/self-provisioners_clusterrolebinding.yaml`, removes the mechanism that previously disabled self-provisioning for all clusters. This is a significant functional change that is not explicitly stated in the commit message and could lead to unintended re-enabling of self-provisioning.
*   **Suggestion**: If disabling self-provisioning is still a requirement, this functionality needs to be restored or replaced. If the intent is to re-enable self-provisioning, this should be clearly documented in the commit message.

### `argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml`

*   **Issue**: The `destination.namespace` is hardcoded to `cert-manager`. It is generally not recommended to deploy unrelated applications into the namespace of another critical component like `cert-manager`.
*   **Suggestion**: Consider deploying `konflux-support-ops` into its own dedicated namespace (e.g., `konflux-support-ops`) or a more appropriate operational namespace.

```diff
--- a/argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml
+++ b/argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml
@@ -26,7 +26,7 @@
         repoURL: https://github.com/redhat-appstudio/infra-deployments.git
         targetRevision: main
       destination:
-        namespace: cert-manager
+        namespace: konflux-support-ops # Or another appropriate dedicated namespace
         server: '{{server}}'
       syncPolicy:
         automated:
  • Issue: The selfHeal policy is set to false. For an operational component like konflux-support-ops, selfHeal: true is often preferred to ensure the application automatically recovers from any drift and maintains its desired state without manual intervention.
  • Suggestion: Reconsider if selfHeal: true would be more appropriate for this component. If selfHeal: false is intentional, add a comment explaining the rationale.
--- a/argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml
+++ b/argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml
@@ -30,7 +30,7 @@
       syncPolicy:
         automated:
           prune: true
-          selfHeal: true
+          selfHeal: true # Consider setting to true for operational components
         syncOptions:
           - CreateNamespace=true
         retry:

components/konflux-support-ops/development/konflux-support-ops-generator.yaml

  • Bug: The repo and namespace fields are left as placeholders (<?>). These must be replaced with actual values for the Helm chart to be correctly identified and deployed.
  • Suggestion: Provide the correct Helm chart repository URL and the target namespace for the Helm release.
--- a/components/konflux-support-ops/development/konflux-support-ops-generator.yaml
+++ b/components/konflux-support-ops/development/konflux-support-ops-generator.yaml
@@ -3,9 +3,9 @@
 metadata:
   name: konflux-support-ops
 name: konflux-support-ops
-repo: <?>
+repo: "https://charts.example.com/konflux-support-ops" # Replace with actual Helm chart repository URL
 version: 0.19.0
-namespace: <?>
+namespace: "konflux-support-ops" # Replace with actual target namespace
 releaseName: konflux-support-ops
 valuesInline:
   resources:

components/konflux-support-ops/staging/konflux-support-ops-generator.yaml

  • Bug: The repo and namespace fields are left as placeholders (<?>). These must be replaced with actual values for the Helm chart to be correctly identified and deployed.
  • Suggestion: Provide the correct Helm chart repository URL and the target namespace for the Helm release.
--- a/components/konflux-support-ops/staging/konflux-support-ops-generator.yaml
+++ b/components/konflux-support-ops/staging/konflux-support-ops-generator.yaml
@@ -3,9 +3,9 @@
 metadata:
   name: konflux-support-ops
 name: konflux-support-ops
-repo: <?>
+repo: "https://charts.example.com/konflux-support-ops" # Replace with actual Helm chart repository URL
 version: 0.19.0
-namespace: <?>
+namespace: "konflux-support-ops" # Replace with actual target namespace
 releaseName: konflux-support-ops
 valuesInline:
   resources:

Copy link
Contributor

Code Review by Gemini

Here's a review of the provided code changes:

### Issues and Suggestions

**1. `argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml`**

*   **Issue**: The `destination.namespace` is set to `cert-manager`. It is highly unlikely that `konflux-support-ops` should be deployed into the `cert-manager` namespace. This could lead to permission issues, resource conflicts, or simply an incorrect logical grouping of components.
    *   **Suggestion**: Change the `namespace` to a more appropriate one for `konflux-support-ops`, such as `konflux-system`, `appstudio-system`, or a dedicated `konflux-support-ops` namespace if it's meant to be isolated.

```diff
--- a/argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml
+++ b/argo-cd-apps/base/member/infra-deployments/konflux-support-ops/konflux-support-ops.yaml
@@ -26,7 +26,7 @@
         repoURL: https://github.com/redhat-appstudio/infra-deployments.git
         targetRevision: main
       destination:
-        namespace: cert-manager
+        namespace: <appropriate-namespace> # e.g., konflux-system or konflux-support-ops
         server: '{{server}}'
       syncPolicy:
         automated:

2. components/konflux-support-ops/development/konflux-support-ops-generator.yaml

  • Issue: The repo and namespace fields are left as placeholders (<?>). These must be filled with actual values for Kustomize to function correctly.
    • Suggestion: Replace <?> with the correct Helm chart repository URL and the target namespace where the Helm chart should be deployed.
--- a/components/konflux-support-ops/development/konflux-support-ops-generator.yaml
+++ b/components/konflux-support-ops/development/konflux-support-ops-generator.yaml
@@ -4,9 +4,9 @@
 metadata:
   name: konflux-support-ops
 name: konflux-support-ops
-repo: <?>
+repo: <helm-chart-repo-url> # e.g., https://charts.example.com/
 version: 0.19.0
-namespace: <?>
+namespace: <target-namespace> # e.g., konflux-system
 releaseName: konflux-support-ops
 valuesInline:
   resources:

3. components/konflux-support-ops/staging/konflux-support-ops-generator.yaml

  • Issue: Similar to the development generator, the repo and namespace fields are left as placeholders (<?>). These must be filled with actual values.
    • Suggestion: Replace <?> with the correct Helm chart repository URL and the target namespace where the Helm chart should be deployed.
--- a/components/konflux-support-ops/staging/konflux-support-ops-generator.yaml
+++ b/components/konflux-support-ops/staging/konflux-support-ops-generator.yaml
@@ -4,9 +4,9 @@
 metadata:
   name: konflux-support-ops
 name: konflux-support-ops
-repo: <?>
+repo: <helm-chart-repo-url> # e.g., https://charts.example.com/
 version: 0.19.0
-namespace: <?>
+namespace: <target-namespace> # e.g., konflux-system
 releaseName: konflux-support-ops
 valuesInline:
   resources:

4. components/konflux-support-ops/staging/kustomization.yaml

  • Issue: The file is missing a newline character at the end. While not a functional bug, it's a common best practice for YAML files and can sometimes cause issues with certain tools or version control systems.
    • Suggestion: Add a newline character at the end of the file.
--- a/components/konflux-support-ops/staging/kustomization.yaml
+++ b/components/konflux-support-ops/staging/kustomization.yaml
@@ -3,4 +3,4 @@
 kind: Kustomization
 
 generators:
-- konflux-support-ops-generator.yaml
+- konflux-support-ops-generator.yaml
\ No newline at end of file

metadata:
name: konflux-support-ops
name: konflux-support-ops
repo: <?>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avi-biton Any idea which repo link should I link here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using helm chart to deploy it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I a not using helm chat. In that case what should I fill in there?

Copy link
Contributor

Choose a reason for hiding this comment

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

If support-ops is not deployed by helm chart, you should not use HelmChartInflationGenerator
Take a look at https://github.com/redhat-appstudio/infra-deployments/tree/main/components/smee-client and use it as an example for how to create deployment from your image

name: konflux-support-ops
repo: <?>
version: 0.19.0
namespace: <?>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avi-biton should I create a new namespace here?

metadata:
name: konflux-support-ops
name: konflux-support-ops
repo: <?>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avi-biton Any idea which repo link should I link here?

name: konflux-support-ops
repo: <?>
version: 0.19.0
namespace: <?>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avi-biton Should I create a new namespace or use an existing one?

Copy link

openshift-ci bot commented Sep 12, 2025

@Kousalya1998: 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/appstudio-e2e-tests 4479d68 link true /test appstudio-e2e-tests
ci/prow/konflux-e2e-v416-optional 4479d68 link false /test konflux-e2e-v416-optional

Full PR test history. Your PR dashboard.

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.

repoURL: https://github.com/redhat-appstudio/infra-deployments.git
targetRevision: main
destination:
namespace: cert-manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the namespace cert-manager?

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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.

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.

3 participants