Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #1189
base: main
Are you sure you want to change the base?
ConfigMap as a ValueSource in Param in TaskRuns and PipelineRuns #1189
Changes from all commits
c34749a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the indentation seems off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question on this: does this mean the would remove
valueFrom
and become avalue
field ? We would loose some information if that's the case.I would rather keep
valueFrom
and have a field calledresolvedValue
(or something) that would contain the value. This way, we do not remove the "user input" that wasvalueFrom
. It adds one more field but it's more explicit and more traceable (we know from where the value came from).cc @tektoncd/core-maintainers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vdemeester. Correct. This means that we are losing this info. I thought about having a
resolvedValue
field but did not propose it because:There is no getter method for param.Value and the field is accessed directly in tons of occurrences across the codebase. A new field
resolvedValue
would require the new getter method + a ton of refactoring. I don't think it is worth it but I can be wrong. Let me know what you think.Some info is already lost in the "sub-objects" of the PipelineRun k8s object. Example: the params that are replaced in the PipelineTasks so I didn't see it as a big issue
Somewhat adjacent to this discussion: if we don't go the getter method route, we can add a private (so that it is not (de)serialized)and cannot be set by the user in the yaml) boolean field
param.valueFrom.isResolved
which is set to true during the value resolution .... and then during the k8s object patching, this will validate that thievalue
was done by the reconciler and not the user who is applying a patched yamlThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working on the draft tektoncd/pipeline#8642 , 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 and to tell the users that params with value sources will always be treated as strings.
Another option is not to allow inline parameters if value source is used (in order to dynamically determine the type of
Param
in the first reconcilehttps://github.com/tektoncd/pipeline/blob/781c2800649e24e1c098daaad95eba4016b86def/pkg/apis/pipeline/v1/taskrun_validation.go#L200
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking again at the code of the function
createParamSpecFromParam
, it seems that these freshly-created ParamSpecs are only used for validation and are not appended to the ParamSpecs array of TaskSpec/PipelineSpec . Maybe what we can do is to:This will not affect the CRD content as these ParamSpecs are never appended to PipelineSpec/TaskSpec anyways. However, we're getting exposed to getting validation errors on these "fresh" ParamSpecs after the TaskRun/PipelineRun creation. These are the validations done
https://github.com/tektoncd/pipeline/blob/23b3deefcea58e469e5775c4b39f6657dc80b538/pkg/apis/pipeline/v1/pipelinerun_validation.go#L218
Another thing worth noting is how this paramspec is being created in
createParamSpecFromParam
paramSpec.Default is a pointer while param.Value is not a pointer. So paramSpec.Default will be a non-null field with an empty value instead of null/empty field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the working group meeting today, it was suggested we can support inline parameters by using the
Type
field. I checked and this field is only in ParamSpec and ParamValue , not in Paramhttps://github.com/tektoncd/pipeline/blob/990917dffc997071df470be66aed30a10338a5e0/pkg/apis/pipeline/v1/param_types.go#L191-L196
What I can do is to add an optional
Type
field inside the ValueSource struct. Then:createParamSpecFromParam
in validation (for inline parameters) , the ParamSpec type will be populated based on the ValueSource type (and will default to string if ValueSource.Type is not defined by the user).ParamValue
instance, the Type in this ParamValue will be validated against (i.e. fail if different than) the Type (if defined) in ValueSource