Skip to content

Conversation

JoaoPedroPP
Copy link
Contributor

Reverts #7417

Copy link
Contributor

Code Review by Gemini

This pull request is a revert of a previous refactor. It aims to restore the Nginx configuration to its prior state, which involves dynamically generating Nginx configuration snippets within an init container rather than using static files managed by Kustomize.

While the revert appears to be functionally correct in restoring the previous behavior, it reintroduces a less declarative and potentially less maintainable approach to Nginx configuration management compared to the refactored state.

Suggested Improvements:

  1. Explicitly Define KUBEARCHIVE_URL for Disabled Environments:
    In several environment-specific kustomization.yaml files, the KUBEARCHIVE_URL literal is omitted when KubeArchive is intended to be disabled. While the current logic correctly handles this by generating a comment in the Nginx config, explicitly setting KUBEARCHIVE_URL="" would improve clarity and consistency across all environment configurations. This makes it immediately clear that KubeArchive is intentionally disabled for that environment.

    File: components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
    Lines to change:

    --- a/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
    +++ b/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
    @@ -19,6 +19,7 @@ configMapGenerator:
       - name: proxy-init-config
         literals:
           - IMPERSONATE=true
           - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
    +      - KUBEARCHIVE_URL=
     
     patches:
       - path: add-service-certs-patch.yaml

    This change should also be applied to the following files for consistency:

    • components/konflux-ui/production/kflux-prd-rh02/kustomization.yaml
    • components/konflux-ui/production/stone-prd-rh01/kustomization.yaml
    • components/konflux-ui/production/stone-prod-p01/kustomization.yaml

Copy link
Contributor

@arewm arewm left a comment

Choose a reason for hiding this comment

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

/lgtm

We saw some breaking changes potentially related to the merging of the original PR on some clusters.

Copy link

openshift-ci bot commented Aug 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arewm, JoaoPedroPP

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

@JoaoPedroPP
Copy link
Contributor Author

/lgtm

Copy link

openshift-ci bot commented Aug 18, 2025

@JoaoPedroPP: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@openshift-merge-bot openshift-merge-bot bot merged commit dff3275 into redhat-appstudio:main Aug 18, 2025
6 checks passed
JoaoPedroPP added a commit to JoaoPedroPP/infra-deployments that referenced this pull request Aug 18, 2025
gcpsoares pushed a commit to gcpsoares/infra-deployments that referenced this pull request Aug 19, 2025
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