Added missing Tekton ConfigMaps and Secrets. #443
Conversation
|
A new image has been built to help with testing out this PR: To use this image run the following: cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/443/head
git checkout -b pullrequest d9a8e749d8e390681810daa83b556413103fc425
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-443"More instructions here on how to deploy and test a Data Science Pipelines Application. |
|
Change to PR detected. A new PR build was completed. |
config/v2/cache/kustomization.yaml
Outdated
| @@ -1,3 +1,5 @@ | |||
| namePrefix: data-science-pipelines-operator- | |||
There was a problem hiding this comment.
Is there a reason why we're adding name Prefixes to individual directories' kustomization.yaml's?
There was a problem hiding this comment.
Okay nvm, read the description.
There was a problem hiding this comment.
Opening this comment. This should not be part of this PR but part of a follow-up issue to fix later
There was a problem hiding this comment.
@rimolive Due to the namePrefix already being in the base kustomization.yaml file, the ConfigMaps and Secrets were inadvertently getting the same prefix which didn't resolve this particular issue. I had no choice but to move all these namePrefix's to the respective kustomization.yaml's to prevent the ConfigMap and Secrets from inheriting the same. Also I researched if there was a possibility to exclude such namePrefix's for said types, but it wasn't designed to skip unfortunately.
There was a problem hiding this comment.
@amadhusu you are right, but @rimolive 's point is valid, let me propose a different solution, add these configmap/secrets here: https://github.com/opendatahub-io/data-science-pipelines-operator/tree/dspv2/config/overlays/make-v2deploy
and not part of the v2 kustomize build, so the annotations are not inherited. This is also useful to separate these out, because we will likely want to remove these later (we don't want to add tekton resources, it's more or less to get the deployment in a working state right now), since these are tekton resources and it's unclear right now why they are not included by openshift-pipelines.
There was a problem hiding this comment.
Oh I missed the fact, there is another base kustomization.yaml which supercedes the v2 kustomize build. I am modifying the PR to include the ConfigMaps and Secrets here instead which makes more sense. Thanks @HumairAK .
config/v2/cache/kustomization.yaml
Outdated
| @@ -1,3 +1,5 @@ | |||
| namePrefix: data-science-pipelines-operator- | |||
There was a problem hiding this comment.
Opening this comment. This should not be part of this PR but part of a follow-up issue to fix later
|
Change to PR detected. A new PR build was completed. |
|
Change to PR detected. A new PR build was completed. |
Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>
|
Change to PR detected. A new PR build was completed. |
| @@ -5,7 +5,6 @@ resources: | |||
| - mutatingwebhookconfig.yaml | |||
| - role.yaml | |||
| - rolebinding.yaml | |||
| - secret.yaml | |||
There was a problem hiding this comment.
@amadhusu the resources here are part of the exithandler/webhook component, this is how the organization is for the v2 directory and we should remain consistent.
Can you make the following changes:
- move this secret back to
config/v2/exithandler/webhook/secret.yaml - same with
config/v2/kfptask/webhook/secret.yaml - move
config/v2/secrets/tektonpipelineloopwebhookcertssecret.yamltoconfig/v2/pipelineloop/webhook/secret.yaml - rename
config/v2/configmapstoconfig/v2/tektonconfigs
There was a problem hiding this comment.
@HumairAK - To clarify, you want me to revert the changes to the locations of these secrets but include these secrets directly via the base kustomization.yaml (https://github.com/opendatahub-io/data-science-pipelines-operator/tree/dspv2/config/overlays/make-v2deploy)?
OR
do you want me to revert the changes to the location of these secrets and also the revert the kustomization.yaml in each webhook subdirectory which would circle us back to the original issue of namePrefix which would result in the bullet points mentioned below?
config/v2/exithandler/webhook/secret.yamlwould becomedata-science-pipeline-operator-kfp-exithandler-webhook-certsafter deployment to OCP instead ofkfp-exithandler-webhook-certsconfig/v2/kfptask/webhook/secret.yamlwould becomedata-science-pipeline-operator-kfptask-webhook-certsafter deployment to OCP instead ofkfptask-webhook-certsconfig/v2/pipelineloop/webhook/secret.yamlwould becomedata-science-pipeline-operator-tektonpipelineloop-webhook-certsinstead oftektonpipelineloop-webhook-certs
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HumairAK 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 |
openshift-merge-bot
bot
merged commit ed4d5c0
into
opendatahub-io:dspv2
The issue resolved by this Pull Request:
Resolves #377
Description of your changes:
Added missing Tekton ConfigMaps and Secrets. Also modified kustomization.yaml files to ensure that namePrefix is added to all the other files except for these specific ConfigMaps and Secrets.
Testing instructions
make v2deploy V2INFRA_NS=${OPERATOR_NS}after setting OPERATOR_NS to the openshift-pipelines namespace.Checklist