Add webhookconfiguration options to tektonConfig additional options #2129

jkhelil · 2024-04-29T15:53:14Z

Changes

Adds webhook configuration options(FailurePolicy, TimeoutSeconds, SideEffects) to tektonConfig additional options

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

Adds webhook configuration options(FailurePolicy, TimeoutSeconds, SideEffects) to tektonConfig additional options

tekton-robot · 2024-04-29T15:53:24Z

Hi @jkhelil. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jkandasa · 2024-04-29T17:37:51Z

/ok-to-test

pkg/apis/operator/v1alpha1/additional_options.go

tekton-robot · 2024-04-29T17:44:56Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-04-30T06:28:15Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-04-30T07:55:21Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-04-30T10:30:15Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-04-30T11:38:32Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

jkandasa · 2024-05-07T05:20:39Z

docs/TektonPipeline.md

+* `deployments` - additional deployments to be created by the operator
+* `statefulsets` - additional statefulsets to be created by the operator
+* `horizontalPodAutoscalers` - additional horizontalPodAutoscalers to be created by the operator
+* `webhookConfigurationOptions` - additional options for pipelines webooks, To get detailed information about webhooks options visit https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/


@jkhelil can you move the documentation under,
https://github.com/tektoncd/operator/blob/main/docs/TektonConfig.md#additional-fields-as-options

https://tekton.dev/docs/operator/tektonconfig/#additional-fields-as-options

pkg/reconciler/common/transformer_additional_options.go

tekton-robot · 2024-05-07T05:22:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkandasa

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

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

tekton-robot · 2024-05-07T06:40:06Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-05-07T07:15:30Z

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/transformer_additional_options.go	87.5%	87.1%	-0.4

jkandasa · 2024-05-08T05:08:37Z

docs/TektonConfig.md

@@ -650,3 +655,12 @@ The following fields are supported in `HorizontalPodAutoscaler` (aka HPA)
 [schedule]:https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/#cron-schedule-syntax
 [priorityClassName]: https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#pod-priority
 [priorityClass]: https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass
+
+### webhookConfigurationOptions


Please use 4 number of ####, so that webhookConfigurationOptions should comes under Options header
Header can be webhookConfigurationOptions => Webhook Configuration Options

also, this needs to be moved little above the links

jkandasa · 2024-05-08T05:09:17Z

docs/TektonPipeline.md

+      validation.webhook.pipeline.tekton.dev:
+        failurePolicy: "Ignore"
+        timeoutSeconds: 20
+        SideEffects: None


please remove the changes from TektonPipeline.md

tekton-robot · 2024-05-22T11:50:54Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	43.2%	0.7
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-05-22T12:08:48Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	43.2%	0.7
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

jkhelil · 2024-05-22T12:30:18Z

@piyush-garg @savitaashture can you have a fresh look please ?

piyush-garg · 2024-05-28T12:08:09Z

pkg/apis/operator/v1alpha1/tektonconfig_validation.go

@@ -115,6 +115,8 @@ func (tc *TektonConfig) Validate(ctx context.Context) (errs *apis.FieldError) {

 	errs = errs.Also(tc.Spec.Pipeline.PipelineProperties.validate("spec.pipeline"))

+	errs = errs.Also(tc.Spec.Pipeline.Options.validate("spec.pipeline.options"))


we should be doing here the config options one, and for others we should in respective valdations. I think we should validate this for all components

piyush-garg · 2024-05-28T12:10:07Z

docs/TektonConfig.md

@@ -655,6 +664,14 @@ The following fields are supported in `HorizontalPodAutoscaler` (aka HPA)

 **NOTE**: If a Deployment or StatefulSet has a Horizontal Pod Autoscaling (HPA) and is in active state, Operator will not control the replicas to that resource. However if `status.desiredReplicas` and `spec.minReplicas` not present in HPA, operator takes the control. Also if HPA disabled, operator takes control. Even though the operator takes the control, the replicas value will be adjusted to the hpa's scaling range.

+#### webhookConfigurationOptions
+Defines additional options for each webhooks. Use webhook name as a key to define options for a webhook. To get detailed information about webhooks options visit https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/


we should add a note for if the webhook does not exist we that name, the configuration will be ignored

comment added

tekton-robot · 2024-05-31T13:50:49Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	44.4%	1.9
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

jkhelil · 2024-05-31T14:10:55Z

@savitaashture @piyush-garg @jkandasa
Can you retake a look/lgtm please ?

tekton-robot · 2024-05-31T14:56:48Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	45.7%	3.1
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

tekton-robot · 2024-06-04T11:01:26Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	45.7%	3.1
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

savitaashture · 2024-06-06T06:56:48Z

docs/TektonConfig.md

+the following options are supported for webhookConfigurationOptions
+* `failurePolicy` -  defines how unrecognized errors and timeout errors from the admission webhook are handled. Allowed values are `Ignore` or `Fail`
+* `timeoutSeconds` - allows configuring how long the API server should wait for a webhook to respond before treating the call as a failure.
+* `sideEffects` -  indicates whether the webhook have a side effet. Allowed values are `None`, `NoneOnDryRun`, `Unknown`, or 'Some'


@jkhelil i see the valid values for sideEffects are None and NoneOnDryRun based on the official k8s doc https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#side-effects

Can you please recheck for Unknown, or 'Some'

I see all the 4 values here https://github.com/kubernetes/api/blob/master/admissionregistration/v1beta1/types.go#L74-L87 but not sure why k8s doc did not list this 🤔

I have picked the values from the code https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/admissionregistration/types.go#L122
and the values seems very old, so I am thinking the doc is not up to date

pkg/apis/operator/v1alpha1/additional_options_validation.go

tekton-robot · 2024-06-06T07:19:02Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	45.7%	3.1
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

savitaashture · 2024-06-06T07:58:37Z

LGTM

Thank you @jkhelil

tekton-robot · 2024-06-06T10:54:57Z

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/apis/operator/v1alpha1/additional_options_validation.go	Do not exist	88.9%
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	42.5%	45.7%	3.1
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go	83.3%	84.2%	0.9
pkg/reconciler/common/transformer_additional_options.go	87.5%	87.1%	-0.4

piyush-garg · 2024-06-07T09:40:05Z

pkg/apis/operator/v1alpha1/tektonconfig_validation.go

+	errs = errs.Also(tc.Spec.Hub.Options.validate("spec.hub.options"))
+	errs = errs.Also(tc.Spec.Dashboard.Options.validate("spec.dashboard.options"))
+	errs = errs.Also(tc.Spec.Chain.Options.validate("spec.chain.options"))
+	errs = errs.Also(tc.Spec.Trigger.Options.validate("spec.trigger.options"))


dont we need to have one for config like errs = errs.Also(tc.Spec.Options.validate("spec.options"))

@piyush-garg TektonConfigSpec struct doestnt have Options

ohk, thanks

piyush-garg · 2024-06-07T12:08:25Z

/lgtm

jkhelil · 2024-06-10T06:54:17Z

/retest

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 29, 2024

tekton-robot requested review from piyush-garg and PuneetPunamiya April 29, 2024 15:53

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 29, 2024

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 29, 2024

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 29, 2024

jkandasa reviewed Apr 29, 2024

View reviewed changes

pkg/apis/operator/v1alpha1/additional_options.go Outdated Show resolved Hide resolved

jkandasa reviewed Apr 29, 2024

View reviewed changes

pkg/apis/operator/v1alpha1/additional_options.go Outdated Show resolved Hide resolved

jkhelil force-pushed the SRVKP-4442 branch from 939d699 to 231fe19 Compare April 30, 2024 06:23

jkhelil force-pushed the SRVKP-4442 branch from 231fe19 to 3b318cd Compare April 30, 2024 07:50

jkhelil force-pushed the SRVKP-4442 branch from 3b318cd to 240bd04 Compare April 30, 2024 10:25

jkhelil force-pushed the SRVKP-4442 branch from 240bd04 to bae8beb Compare April 30, 2024 11:33

jkandasa approved these changes May 7, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2024

jkhelil force-pushed the SRVKP-4442 branch 2 times, most recently from 1f29b82 to 8c8cb88 Compare May 7, 2024 06:33

jkhelil force-pushed the SRVKP-4442 branch from 8c8cb88 to d1b078e Compare May 7, 2024 07:09

jkandasa reviewed May 8, 2024

View reviewed changes

jkhelil force-pushed the SRVKP-4442 branch from 2980c28 to 87a107d Compare May 22, 2024 11:44

jkhelil force-pushed the SRVKP-4442 branch from 87a107d to d752e16 Compare May 22, 2024 12:01

piyush-garg reviewed May 28, 2024

View reviewed changes

jkhelil force-pushed the SRVKP-4442 branch from d752e16 to fef391e Compare May 31, 2024 13:45

jkhelil force-pushed the SRVKP-4442 branch 2 times, most recently from 1f934c0 to 7fc18d5 Compare May 31, 2024 14:50

jkhelil force-pushed the SRVKP-4442 branch from 7fc18d5 to 2d52be0 Compare June 4, 2024 10:44

savitaashture reviewed Jun 6, 2024

View reviewed changes

pkg/apis/operator/v1alpha1/additional_options_validation.go Outdated Show resolved Hide resolved

jkhelil force-pushed the SRVKP-4442 branch from 2d52be0 to f960f5e Compare June 6, 2024 07:12

Add webhookconfiguration options to tektonConfig additional options

2559191

jkhelil force-pushed the SRVKP-4442 branch from f960f5e to 2559191 Compare June 6, 2024 10:48

piyush-garg reviewed Jun 7, 2024

View reviewed changes

tekton-robot assigned piyush-garg Jun 7, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2024

tekton-robot merged commit 8b91274 into tektoncd:main Jun 10, 2024
8 checks passed

jkhelil deleted the SRVKP-4442 branch September 2, 2024 11:02

		@@ -115,6 +115,8 @@ func (tc TektonConfig) Validate(ctx context.Context) (errs apis.FieldError) {

		errs = errs.Also(tc.Spec.Pipeline.PipelineProperties.validate("spec.pipeline"))

		errs = errs.Also(tc.Spec.Pipeline.Options.validate("spec.pipeline.options"))

Add webhookconfiguration options to tektonConfig additional options #2129

Add webhookconfiguration options to tektonConfig additional options #2129

Conversation

jkhelil commented Apr 29, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 29, 2024

jkandasa commented Apr 29, 2024

tekton-robot commented Apr 29, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 7, 2024

tekton-robot commented May 7, 2024

tekton-robot commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 22, 2024

tekton-robot commented May 22, 2024

jkhelil commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 31, 2024

jkhelil commented May 31, 2024

tekton-robot commented May 31, 2024

tekton-robot commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 6, 2024

savitaashture commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-garg commented Jun 7, 2024

jkhelil commented Jun 10, 2024

jkhelil commented Apr 29, 2024 •

edited

Loading