New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix to deploy TektonHub in custom namespace #1865

Merged

tekton-robot merged 1 commit into tektoncd:main from pratap0007:fix-tknhub-ns

Feb 8, 2024

Contributor

pratap0007 commented Nov 21, 2023 •

edited by jkandasa

Loading

Changes

This will add the fix for targetnamespace deletion
it will delete the namespace only if namespace contains
ownerreference of same reconcilation

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Run make test lint before submitting a PR
Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Fix targetnamespace deletion of Tekton Hub

tekton-robot added the release-note label

tekton-robot requested review from piyush-garg and PuneetPunamiya

November 21, 2023 18:09

tekton-robot added the size/S label

Contributor

tekton-robot commented Nov 21, 2023

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/targetnamespace.go	86.4%	89.3%	2.8

vdemeester approved these changes

View reviewed changes

Member

vdemeester left a comment

/cc @jkandasa @concaf

tekton-robot requested review from concaf and jkandasa

November 22, 2023 09:26

Contributor

tekton-robot commented Nov 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

tekton-robot added the approved label

Member

jkandasa commented Nov 22, 2023

/hold
@vdemeester @pratap0007 we are keeping only targetNamespace and deleting all other namespaces which has labels that matches to targetNamespace, Those was used as target namespace.

With this PR we are removing the deletion part completely. I need sometime to verify this change.

Thanks for the PR @pratap0007

tekton-robot added the do-not-merge/hold label

Contributor Author

pratap0007 commented Nov 22, 2023

/hold @vdemeester @pratap0007 we are keeping only targetNamespace and deleting all other namespaces which has labels that matches to targetNamespace, Those was used as target namespace.

With this PR we are removing the deletion part completely. I need sometime to verify this change.

Thanks for the PR @pratap0007

why do we want to delete all other namespaces other than targetNamespace ?

pratap0007 force-pushed the fix-tknhub-ns branch 2 times, most recently from 4632397 to 9290eba Compare

January 22, 2024 09:42

Contributor

tekton-robot commented Jan 22, 2024

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/targetnamespace.go	86.4%	89.3%	2.8

pratap0007 force-pushed the fix-tknhub-ns branch 3 times, most recently from 4f3fc26 to d18c7ef Compare

February 6, 2024 20:30

Contributor Author

pratap0007 commented Feb 7, 2024

/remove-hold

tekton-robot removed the do-not-merge/hold label

jkandasa reviewed

View reviewed changes

pkg/reconciler/kubernetes/tektonhub/tektonhub.go Outdated Show resolved Hide resolved

pratap0007 force-pushed the fix-tknhub-ns branch from d18c7ef to cfe514b Compare

February 7, 2024 11:45

tekton-robot added size/M and removed size/S labels

PuneetPunamiya reviewed

View reviewed changes

pkg/reconciler/kubernetes/tektonhub/tektonhub.go Outdated

+              					ObjectMeta: metav1.ObjectMeta{
+              						Name: th.Spec.TargetNamespace,
+              					},
+              				}

Member

PuneetPunamiya Feb 7, 2024

I think we need to add the ownerRef as well
cc @jkandasa @piyush-garg

jkandasa reviewed

View reviewed changes

pkg/reconciler/kubernetes/tektonhub/tektonhub.go Outdated Show resolved Hide resolved

pratap0007 force-pushed the fix-tknhub-ns branch from cfe514b to 14c7d81 Compare

February 7, 2024 16:00

tekton-robot added size/S and removed size/M labels

PuneetPunamiya reviewed

View reviewed changes

pkg/reconciler/kubernetes/tektonhub/tektonhub.go Outdated

-              			"targetNamespace", th.Spec.GetTargetNamespace(),
-              			err,
-              		)
+              	tc, _ := r.operatorClientSet.OperatorV1alpha1().TektonConfigs().Get(ctx, v1alpha1.ConfigResourceName, metav1.GetOptions{})

Member

PuneetPunamiya Feb 7, 2024

I still feel this would not work when TektonConfig is nil

Contributor Author

pratap0007 Feb 7, 2024

can you please explain the case when TektonConfig could be nil, so I can check once this ?

Member

PuneetPunamiya Feb 8, 2024

Meaning TektonHub is independent of TekttonConfig, so ideally we should not check for the namespace of TektonConfig, we can directly check if the namespace mentioned in TektonHub is present or not and based on that we can directly create that

For ref: 51fff50#diff-1c3da4772ce1a38a45935ea0814741d91d7803c96d9aef2b0f10b00f6d8b33f3L864-L875

Contributor Author

pratap0007 Feb 8, 2024

okay, thank you

pratap0007 force-pushed the fix-tknhub-ns branch 2 times, most recently from c075649 to 5ddd585 Compare

February 8, 2024 03:37

jkandasa reviewed

View reviewed changes

pkg/reconciler/kubernetes/tektonhub/tektonhub.go Outdated Show resolved Hide resolved

pratap0007 force-pushed the fix-tknhub-ns branch from 5ddd585 to 540be87 Compare

February 8, 2024 10:53

tekton-robot added size/XS and removed size/S labels


          Fix targetnamespace deletion

6b32b71

This will add the fix for targetnamespace deletion
it will delete the namespace only if namespace contains
ownerreference of same reconcilation

Signed-off-by: Shiv Verma<[email protected]>

pratap0007 force-pushed the fix-tknhub-ns branch from 540be87 to 6b32b71 Compare

February 8, 2024 11:21

tekton-robot added size/M and removed size/XS labels

Contributor

tekton-robot commented Feb 8, 2024

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/targetnamespace.go	86.4%	86.9%	0.4

Member

jkandasa commented Feb 8, 2024

/test pull-tekton-operator-integration-tests

Member

jkandasa commented Feb 8, 2024

/lgtm

tekton-robot assigned jkandasa

tekton-robot added the lgtm label

tekton-robot merged commit 223708d into tektoncd:main

8 checks passed

Contributor Author

pratap0007 commented Mar 13, 2024

/cherry-pick release-v0.69.x

Contributor

tekton-robot commented Mar 13, 2024

@pratap0007: new pull request created: #2050

In response to this:

/cherry-pick release-v0.69.x

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/test-infra repository.

tekton-robot mentioned this pull request

[release-v0.69.x] Fix to deploy TektonHub in custom namespace #2050

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

PuneetPunamiya PuneetPunamiya left review comments

jkandasa jkandasa left review comments

vdemeester vdemeester approved these changes

piyush-garg Awaiting requested review from piyush-garg

concaf Awaiting requested review from concaf

Labels

approved lgtm release-note size/M