Added missing Tekton ConfigMaps and Secrets. #443

amadhusu · 2023-11-06T17:25:26Z

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

Ensure the OCP Cluster you test on has OpenShift Pipelines Operator (v1.12.0+) installed.
Run make v2deploy V2INFRA_NS=${OPERATOR_NS} after setting OPERATOR_NS to the openshift-pipelines namespace.
Ensure the specific ConfigMaps and Secrets are created as per screenshots below:

Ensure all the webhook pods are up and running without throwing any errors in the logs of missing the respective configmaps and secrets.

Checklist

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

dsp-developers · 2023-11-06T17:27:51Z

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-443
An OCP cluster where you are logged in as cluster admin is required.

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.

dsp-developers · 2023-11-06T17:34:07Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-443

DharmitD · 2023-11-06T17:37:11Z

config/v2/cache/kustomization.yaml

@@ -1,3 +1,5 @@
+namePrefix: data-science-pipelines-operator-


Is there a reason why we're adding name Prefixes to individual directories' kustomization.yaml's?

Okay nvm, read the description.

Opening this comment. This should not be part of this PR but part of a follow-up issue to fix later

@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.

@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.

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 .

rimolive

/lgtm

rimolive

Just to summarize: we should not make any changes on the namePrefix attribute. Just make sure all the congimaps/secrets are present in this PR.

rimolive · 2023-11-08T19:06:02Z

config/v2/cache/kustomization.yaml

@@ -1,3 +1,5 @@
+namePrefix: data-science-pipelines-operator-


Opening this comment. This should not be part of this PR but part of a follow-up issue to fix later

config/v2/driver/kustomization.yaml

config/v2/exithandler/controller/kustomization.yaml

config/v2/exithandler/crd/kustomization.yaml

config/v2/kfptask/controller/kustomization.yaml

config/v2/pipelineloop/controller/kustomization.yaml

config/v2/pipelineloop/crd/kustomization.yaml

config/v2/pipelineloop/leaderelection/kustomization.yaml

config/v2/pipelineloop/webhook/kustomization.yaml

config/v2/tektoncrds/kustomization.yaml

dsp-developers · 2023-11-10T09:27:53Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-443

dsp-developers · 2023-11-10T09:30:11Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-443

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

dsp-developers · 2023-11-10T09:44:16Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-443

HumairAK · 2023-11-10T15:36:53Z

config/v2/exithandler/webhook/kustomization.yaml

@@ -5,7 +5,6 @@ resources:
 - mutatingwebhookconfig.yaml
 - role.yaml
 - rolebinding.yaml
- secret.yaml


@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.yaml to config/v2/pipelineloop/webhook/secret.yaml

rename config/v2/configmaps to config/v2/tektonconfigs

@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.yaml would become data-science-pipeline-operator-kfp-exithandler-webhook-certs after deployment to OCP instead of kfp-exithandler-webhook-certs

config/v2/kfptask/webhook/secret.yaml would become data-science-pipeline-operator-kfptask-webhook-certs after deployment to OCP instead of kfptask-webhook-certs

config/v2/pipelineloop/webhook/secret.yaml would become data-science-pipeline-operator-tektonpipelineloop-webhook-certs instead of tektonpipelineloop-webhook-certs

HumairAK · 2023-11-10T17:15:16Z

/lgtm
/approve

openshift-ci · 2023-11-10T17:15:24Z

[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:

~~OWNERS~~ [HumairAK]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from gmfrasca and gregsheremeta November 6, 2023 17:25

amadhusu force-pushed the am/377 branch from d9a8e74 to f4f0957 Compare November 6, 2023 17:30

DharmitD reviewed Nov 6, 2023

View reviewed changes

amadhusu linked an issue Nov 6, 2023 that may be closed by this pull request

[DSPv2] Add/manage missing empty tekton config CMs/Secrets #377

Closed

amadhusu removed a link to an issue Nov 6, 2023

[DSPv2] Add/manage missing empty tekton config CMs/Secrets #377

Closed

rimolive reviewed Nov 7, 2023

View reviewed changes

openshift-ci bot assigned rimolive Nov 7, 2023

openshift-ci bot added the lgtm label Nov 7, 2023

rimolive requested changes Nov 8, 2023

View reviewed changes

openshift-ci bot removed the lgtm label Nov 8, 2023

amadhusu force-pushed the am/377 branch from f4f0957 to 6718903 Compare November 10, 2023 09:24

amadhusu force-pushed the am/377 branch from 6718903 to b7c0251 Compare November 10, 2023 09:28

Added missing Tekton ConfigMaps and Secrets.

bea760e

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

amadhusu force-pushed the am/377 branch from b7c0251 to bea760e Compare November 10, 2023 09:41

HumairAK reviewed Nov 10, 2023

View reviewed changes

openshift-ci bot assigned HumairAK Nov 10, 2023

openshift-ci bot added the lgtm label Nov 10, 2023

openshift-ci bot added the approved label Nov 10, 2023

openshift-merge-bot bot merged commit ed4d5c0 into opendatahub-io:dspv2 Nov 10, 2023
5 checks passed

rimolive mentioned this pull request Nov 13, 2023

[DSPv2] Add/manage missing empty tekton config CMs/Secrets #377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added missing Tekton ConfigMaps and Secrets. #443

Added missing Tekton ConfigMaps and Secrets. #443

amadhusu commented Nov 6, 2023 •

edited

Loading

dsp-developers commented Nov 6, 2023

dsp-developers commented Nov 6, 2023

DharmitD Nov 6, 2023

DharmitD Nov 6, 2023

rimolive Nov 8, 2023

amadhusu Nov 9, 2023

HumairAK Nov 9, 2023 •

edited

Loading

amadhusu Nov 10, 2023 •

edited

Loading

rimolive left a comment

rimolive left a comment

rimolive Nov 8, 2023

dsp-developers commented Nov 10, 2023

dsp-developers commented Nov 10, 2023

dsp-developers commented Nov 10, 2023

HumairAK Nov 10, 2023

amadhusu Nov 10, 2023 •

edited

Loading

HumairAK commented Nov 10, 2023

openshift-ci bot commented Nov 10, 2023

Added missing Tekton ConfigMaps and Secrets. #443

Added missing Tekton ConfigMaps and Secrets. #443

Conversation

amadhusu commented Nov 6, 2023 • edited Loading

The issue resolved by this Pull Request:

Description of your changes:

Testing instructions

Checklist

dsp-developers commented Nov 6, 2023

dsp-developers commented Nov 6, 2023

DharmitD Nov 6, 2023

Choose a reason for hiding this comment

DharmitD Nov 6, 2023

Choose a reason for hiding this comment

rimolive Nov 8, 2023

Choose a reason for hiding this comment

amadhusu Nov 9, 2023

Choose a reason for hiding this comment

HumairAK Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

amadhusu Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

rimolive left a comment

Choose a reason for hiding this comment

rimolive left a comment

Choose a reason for hiding this comment

rimolive Nov 8, 2023

Choose a reason for hiding this comment

dsp-developers commented Nov 10, 2023

dsp-developers commented Nov 10, 2023

dsp-developers commented Nov 10, 2023

HumairAK Nov 10, 2023

Choose a reason for hiding this comment

amadhusu Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

HumairAK commented Nov 10, 2023

openshift-ci bot commented Nov 10, 2023

amadhusu commented Nov 6, 2023 •

edited

Loading

HumairAK Nov 9, 2023 •

edited

Loading

amadhusu Nov 10, 2023 •

edited

Loading

amadhusu Nov 10, 2023 •

edited

Loading