ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #8642

mostafaCamel · 2025-03-12T22:54:12Z

Added a new field ValueSource to v1.Param
Added validation to v1 CRD for Param and ParamValue based on a new feature flga enable-valuefrom-in-param
Added new methods to the reconciler in order to resolve these vallue sources during the first reconcile then upate the k8s resource
Added documentation and integration tests for the new feature
Fixed preexisting unit tests that were failing before the new validation (the params in the test cases were set up incorrectly)

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

- Added a new field ValueSource to v1.Param - Added validation to v1 CRD for Param and ParamValue based on a new feature flga `enable-valuefrom-in-param` - Added new methods to the reconciler in order to resolve these vallue sources during the first reconcile then upate the k8s resource - Added documentation and integration tests for the new feature - Fixed preexisting unit tests that were failing before the new validation (the params in the test cases were set up incorrectly) Signed-off-by: Mostafa Abdelwahab <[email protected]>

tekton-robot · 2025-03-12T22:54:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vdemeester after the PR has been reviewed.
You can assign the PR to them by writing /assign @vdemeester in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

mostafaCamel · 2025-03-12T22:55:36Z

Related discussion about the feature #1744
(Unmerged) TEP tektoncd/community#1189

tekton-robot · 2025-03-12T23:04:04Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	95.2%	95.2%	0.1
pkg/apis/pipeline/v1/param_types.go	98.9%	99.0%	0.1
pkg/apis/pipeline/v1/pipeline_validation.go	99.3%	99.0%	-0.2
pkg/apis/pipeline/v1/pipelinerun_types.go	92.5%	93.1%	0.7
pkg/apis/pipeline/v1/pipelinerun_validation.go	94.7%	94.7%	0.1
pkg/apis/pipeline/v1/taskrun_types.go	82.8%	85.1%	2.3
pkg/apis/pipeline/v1/taskrun_validation.go	97.4%	97.5%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	91.7%	89.8%	-2.0
pkg/reconciler/taskrun/resources/apply.go	99.4%	98.9%	-0.5
pkg/reconciler/taskrun/taskrun.go	86.1%	83.8%	-2.3

mostafaCamel · 2025-03-12T23:06:18Z

Few notes:

After I rebased my branch from main, ko apply -R -f config/ started to fail and I had to use ko create -R -f config/ (with the usual caveats/disadvantages of using create and not being able to iterate new changes with apply subsequently)

The CustomResourceDefinition "taskruns.tekton.dev" is invalid: metadata.annotations: Too long: may not be more than 262144 bytes
Error: error executing 'kubectl apply': exit status 1

This is probably due to the recent script change of adding the OpenAPI schema to the CRD yamls

pipeline/hack/update-codegen.sh

Line 114 in 781c280

${REPO_ROOT_DIR}/hack/update-schemas.sh

While writing integration tests, I realized the new feature is not working for inline parameters. This is due to the following function createParamSpecFromParam which expects a Param to have a Type during validation (which is not the case for parameters with value sources) What I could do is to change the behavior of this function so that it defaults to Type string when populating a ParamSpec from a param with ValueSource

pipeline/pkg/apis/pipeline/v1/taskrun_validation.go

Line 200 in 781c280

func createParamSpecFromParam(p Param, paramSpecForValidation map[string]ParamSpec) map[string]ParamSpec {

mostafaCamel · 2025-03-12T23:08:00Z

/kind feature

mostafaCamel · 2025-03-12T23:16:24Z

There are many files in the PR. Here are the "entrypoints" of the code changes

in ValidateParameters (which is called during TaskRun and Pipeline validation) , calling params.ValidateValueAndValueSourceInEachParam . This function (implementation in pg=kg/apis/v1/pipeline/param_types.go) validates that ValueSource is only used if the feaure flag is enabled + additional structure validation about these fields https://github.com/tektoncd/pipeline/pull/8642/files#diff-e6ceebad50db85ba0dae540a07832bc1692e39213e8592a2d9c3099cdd25fbf0R293
In reconciler/pipelinerun.go (and in the corresponding taskrun reconciler as well), resolve these value sources in the reconcile method https://github.com/tektoncd/pipeline/pull/8642/files#diff-fcdbb0e0ea64e13920b270bf5cb2ca8993c040d77a6173ec004d1795e9e20135R499
In reconciler/pipelinerun.go (and in the corresponding taskrun reconciler as well), within ReconcileKind and after the reconcile call, patch the k8s object with the resolved values (in the spec not in the status) https://github.com/tektoncd/pipeline/pull/8642/files#diff-fcdbb0e0ea64e13920b270bf5cb2ca8993c040d77a6173ec004d1795e9e20135R276

waveywaves · 2025-03-13T04:53:28Z

/assign

waveywaves · 2025-03-13T07:26:41Z

@mostafaCamel can this PR be broken down into multiple smaller PRs? maybe add this feature initially to taskruns and then in another PR to pipeline runs ? also, I see that the related TEP is not merged and hasn't had any discussion (at least in the comments on github). It would be advisable to discuss this in the community call as well if you would like to move this along faster. Thank you for the early PR though, definitely helps the community to understand the feature from a technical standpoint a lot better.

waveywaves · 2025-03-13T07:30:22Z

Just skimmed through #1744 and have better context of this. It would be good to merge the TEP and move it to implementable state before we get back to the PR.

mostafaCamel · 2025-03-13T08:44:54Z

Sounds good. Thanks Vibhav. I can wait until the TEP is merged then will break down the PR. I can also further break it into a pull request for api/ webhook validation (which will also generate the swagger and openapi changes), then a PR for taskrun reconciler (+ integration tests) then a PR for pipelinerun reconciler (+integration tests ) and a final PR for documentation

mostafaCamel · 2025-03-12T23:19:33Z

pkg/apis/pipeline/v1/pipelinerun_types.go

@@ -690,3 +693,21 @@ type PipelineTaskRunTemplate struct {
 	// +optional
 	ServiceAccountName string `json:"serviceAccountName,omitempty"`
 }
+
+// returns true if the RunSpecs are different in params (by value source resolution only) and deeply eual otherwise
+func (baseline *PipelineRunSpec) IsDifferentFromDesiredStateOnlyByValueSourceResolutionInParams(new *PipelineRunSpec) bool {


This is my first PR to this repo so I am not sure if I should create a go interface with DeepCopy() and other methods and fields used here so that I avoid duplicating the code. There is a method that does the same thing for TaskRunSpec https://github.com/tektoncd/pipeline/pull/8642/files#diff-f6f395c4b788e7dad3da4c944d41e8b7e03540f0b49735417423ade04da8462bR532

mostafaCamel · 2025-03-12T23:21:59Z

pkg/apis/pipeline/v1/taskrun_validation_test.go

+				FeatureFlags: &config.FeatureFlags{EnableValueFromInParam: tc.enableValueFromInParam},
+				Defaults:     &config.Defaults{},
+			})
+			err := v1.ValidateParameters(ctx, tc.params)


Ideally I would have tested in this unit test that the method ValidateValueAndValueSourceInEachParam was called but go does not provide mocks (unlike python)

https://github.com/tektoncd/pipeline/pull/8642/files#diff-e6ceebad50db85ba0dae540a07832bc1692e39213e8592a2d9c3099cdd25fbf0R293

mostafaCamel · 2025-03-12T23:27:28Z

pkg/reconciler/pipelinerun/pipelinerun.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal Params patch bytes: %w", err)
+		}
+		return c.PipelineClientSet.TektonV1().PipelineRuns(pr.Namespace).Patch(ctx, pr.Name, types.JSONPatchType, patchParamBytes, metav1.PatchOptions{}, "")


I haven't been able to unit test the reconciler changes because of Patch: the unit test was failing with requeue after: 27969h18m22.144989s . When I tried to use Update instead, the unit test failed with Returned an error {"targetMethod": "ReconcileKind", "error": "Operation cannot be fulfilled on taskruns.tekton.dev \"test-task-param-run-valuesource-patch\": resourceVersion mismatch, got: 00001, wanted: 00003"}

I probably need to fix this testing behavior to add support for patching / non-status Update in

pipeline/test/controller.go

Line 131 in 781c280

return func(action ktesting.Action) (bool, runtime.Object, error) {

mostafaCamel · 2025-03-12T23:30:21Z

test/valuesource_param_test.go

+				t.Fatalf("Succeeded in creating PipelineRun `%s` with valuesource in params even though the feature flag is turned off", pipelineRun.Name)
+			}
+
+			updateConfigMap(ctx, c.KubeClient, system.Namespace(), config.GetFeatureFlagsConfigName(), map[string]string{


I am assuming that this does not affect other integration tests due to the teardown procedure + there is already a separate namespace per integration test anyways

mostafaCamel · 2025-03-12T23:32:01Z

examples/v1/pipelineruns/alpha/param-enum.yaml

@@ -20,8 +20,6 @@ metadata:
 spec:
  params:
  - name: message
-    enum: ["v1", "v2"]


Changed made by mistake. I will undo it in my next amended commit

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. labels Mar 12, 2025

tekton-robot requested review from afrittoli and vdemeester March 12, 2025 22:54

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 12, 2025

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 12, 2025

mostafaCamel mentioned this pull request Mar 12, 2025

Feature Request: Using ConfigMap Values For Params #1744

Open

tekton-robot assigned waveywaves Mar 13, 2025

mostafaCamel mentioned this pull request Mar 19, 2025

ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns tektoncd/community#1189

Open

mostafaCamel commented Mar 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #8642

ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #8642

mostafaCamel commented Mar 12, 2025

tekton-robot commented Mar 12, 2025

mostafaCamel commented Mar 12, 2025

tekton-robot commented Mar 12, 2025

mostafaCamel commented Mar 12, 2025 •

edited

Loading

mostafaCamel commented Mar 12, 2025

mostafaCamel commented Mar 12, 2025

waveywaves commented Mar 13, 2025

waveywaves commented Mar 13, 2025

waveywaves commented Mar 13, 2025

mostafaCamel commented Mar 13, 2025 •

edited

Loading

mostafaCamel Mar 12, 2025

mostafaCamel Mar 12, 2025

mostafaCamel Mar 12, 2025

mostafaCamel Mar 12, 2025

mostafaCamel Mar 12, 2025

ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #8642

Are you sure you want to change the base?

ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #8642

Conversation

mostafaCamel commented Mar 12, 2025

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 12, 2025

mostafaCamel commented Mar 12, 2025

tekton-robot commented Mar 12, 2025

mostafaCamel commented Mar 12, 2025 • edited Loading

mostafaCamel commented Mar 12, 2025

mostafaCamel commented Mar 12, 2025

waveywaves commented Mar 13, 2025

waveywaves commented Mar 13, 2025

waveywaves commented Mar 13, 2025

mostafaCamel commented Mar 13, 2025 • edited Loading

mostafaCamel Mar 12, 2025

Choose a reason for hiding this comment

mostafaCamel Mar 12, 2025

Choose a reason for hiding this comment

mostafaCamel Mar 12, 2025

Choose a reason for hiding this comment

mostafaCamel Mar 12, 2025

Choose a reason for hiding this comment

mostafaCamel Mar 12, 2025

Choose a reason for hiding this comment

mostafaCamel commented Mar 12, 2025 •

edited

Loading

mostafaCamel commented Mar 13, 2025 •

edited

Loading