pipeline icon indicating copy to clipboard operation
pipeline copied to clipboard
verified publisher icon tektoncd

ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns

Open mostafaCamel opened this issue 9 months ago • 12 comments

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

  • [x] Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • [x] Has Tests included if any functionality added or changed
  • [x] 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

mostafaCamel avatar Mar 12 '25 22:03 mostafaCamel

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

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

tekton-robot avatar Mar 12 '25 22:03 tekton-robot

Related discussion about the feature https://github.com/tektoncd/pipeline/issues/1744 (Unmerged) TEP https://github.com/tektoncd/community/pull/1189

mostafaCamel avatar Mar 12 '25 22:03 mostafaCamel

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

tekton-robot avatar Mar 12 '25 23:03 tekton-robot

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 https://github.com/tektoncd/pipeline/blob/781c2800649e24e1c098daaad95eba4016b86def/hack/update-codegen.sh#L114

  • 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 https://github.com/tektoncd/pipeline/blob/781c2800649e24e1c098daaad95eba4016b86def/pkg/apis/pipeline/v1/taskrun_validation.go#L200

mostafaCamel avatar Mar 12 '25 23:03 mostafaCamel

/kind feature

mostafaCamel avatar Mar 12 '25 23:03 mostafaCamel

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

mostafaCamel avatar Mar 12 '25 23:03 mostafaCamel

/assign

waveywaves avatar Mar 13 '25 04:03 waveywaves

@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 avatar Mar 13 '25 07:03 waveywaves

Just skimmed through https://github.com/tektoncd/pipeline/issues/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.

waveywaves avatar Mar 13 '25 07:03 waveywaves

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 avatar Mar 13 '25 08:03 mostafaCamel

@mostafaCamel: PR needs rebase.

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 avatar Mar 28 '25 21:03 tekton-robot

A very practical feature, looking forward to merging it soon. Thank you to the @mostafaCamel for sharing.

l-qing avatar Jun 15 '25 00:06 l-qing

Closing this PR as the associated TEP PR has not been merged yet (there are still unanswered questions about the design. The discussion will be continued in https://github.com/tektoncd/pipeline/issues/1744

mostafaCamel avatar Jul 07 '25 02:07 mostafaCamel