update pipelines deployment in development and staging #7605
Conversation
aThorp96
commented
Aug 12, 2025
aThorp96
commented
Code Review by GeminiThe following issues and improvements are identified in the code changes: Issues
Improvements
|
aThorp96
force-pushed
the
update-osp-dev-stage
branch
from
a58eaa0 to
b99d5e4
Compare
Code Review by Gemini## Code Review
### Issues and Bugs
* **Inconsistent Environment Variable Configuration:** The `GIT_SSL_CAINFO` environment variable is added to the `tekton-pipelines-remote-resolvers` controller in `production/base/main-pipeline-service-configuration.yaml` and `staging/base/main-pipeline-service-configuration.yaml`, but it is missing from `development/main-pipeline-service-configuration.yaml`. If this environment variable is required for proper functioning (as implied by the comment), it should be consistently applied across all environments.
**File: `components/pipeline-service/development/main-pipeline-service-configuration.yaml`**
```diff
--- a/components/pipeline-service/development/main-pipeline-service-configuration.yaml
+++ b/components/pipeline-service/development/main-pipeline-service-configuration.yaml
@@ -1909,6 +1909,10 @@ spec:
tekton-pipelines-remote-resolvers:
spec:
replicas: 4
template:
spec:
containers:
- name: controller
env:
+ # TODO: this is resolved in next operator release and should be removed when it's deployed
+ # https://github.com/tektoncd/operator/commit/f5941cd59f6c7724b2c9d3ad84d11e8a425d2738
+ - name: GIT_SSL_CAINFO
+ value: /tekton-custom-certs/ca-bundle.crt
resources:
limits:
memory: 10Gi
requests:
cpu: "500m"
memory: 10Gi
tekton-pipelines-controller:
```
### Improvements
* **Removal of Explicit Image Overrides:** The removal of `IMAGE_ADDONS_TKN_CLI_SERVE`, `IMAGE_PAC_PAC_CONTROLLER`, `IMAGE_PAC_PAC_WATCHER`, and `IMAGE_PAC_PAC_WEBHOOK` environment variables from the `Subscription` configuration in `development` and `staging` environments is a positive change. This suggests that the updated `pipelines-index-4.15` operator bundle now correctly includes and manages these images by default, reducing the need for manual overrides and simplifying the configuration. No specific changes are suggested here, as this appears to be an intended and beneficial simplification.
### General Observations
* The `CatalogSource` image digest has been correctly updated to `quay.io/openshift-pipeline/pipelines-index-4.15@sha256:6ddc8dfedc41431f7ff577ca93aecf2554dabf20965d05d6e0f512728cdf9b76` across all modified files (`development`, `staging/base`, `staging/stone-stage-p01`, `staging/stone-stg-rh01`), which aligns with the commit body. This is the primary purpose of the commit and is correctly implemented.
|
|
/lgtm |
|
/retest |
Code Review by Gemini## Code Review
### Bug/Issue
**File: `components/pipeline-service/development/main-pipeline-service-configuration.yaml`**
The `GIT_SSL_CAINFO` environment variable, which is added as a workaround in `production` and `staging` environments, is missing from the `development` environment. If this workaround is necessary for the Tekton Operator, it should be applied consistently across all environments where the operator is deployed to prevent potential issues in development.
**Exact lines to change:**
```diff
--- a/components/pipeline-service/development/main-pipeline-service-configuration.yaml
+++ b/components/pipeline-service/development/main-pipeline-service-configuration.yaml
@@ -1909,6 +1909,10 @@ spec:
template:
spec:
containers:
- name: controller
env:
+ # TODO: this is resolved in next operator release and should be removed when it's deployed
+ # https://github.com/tektoncd/operator/commit/f5941cd59f6c7724b2c9d3ad84d11e8a425d2738
- name: GIT_SSL_CAINFO
value: /tekton-custom-certs/ca-bundle.crt
resources:
limits:
memory: 10Gi |
|
/approve |
|
@aThorp96: The following tests failed, say
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. |
|
/rebase |
aThorp96
force-pushed
the
update-osp-dev-stage
branch
from
0bddac8 to
b7e62f7
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aThorp96, gbenhaim, rcerven 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 |
Code Review by GeminiThe provided diff is empty, which means there are no code changes to review. The commit body contains a log of image information (image names, SHA256 hashes, and associated commit IDs), which appears to be output from a script like To perform a code review, please provide the actual code changes (e.g., configuration files, scripts, or manifests) that correspond to the "update pipelines deployment" mentioned in the commit title. Without code, I cannot identify issues, suggest improvements, or specify lines to change. |
