-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[TEP-0076] Add array support for emitting results #4818
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
kind: Task | ||
apiVersion: tekton.dev/v1beta1 | ||
metadata: | ||
name: write-array | ||
annotations: | ||
description: | | ||
A simple task that writes array | ||
spec: | ||
results: | ||
- name: array-results | ||
type: array | ||
description: The array results | ||
steps: | ||
- name: write-array | ||
image: bash:latest | ||
script: | | ||
#!/usr/bin/env bash | ||
echo -n "[\"hello\",\"world\"]" | tee $(results.array-results.path) | ||
- name: check-results-array | ||
image: ubuntu | ||
script: | | ||
#!/bin/bash | ||
VALUE=$(cat $(results.array-results.path)) | ||
EXPECTED=[\"hello\",\"world\"] | ||
diff=$(diff <(printf "%s\n" "${VALUE[@]}") <(printf "%s\n" "${EXPECTED[@]}")) | ||
if [[ -z "$diff" ]]; then | ||
echo "TRUE" | ||
else | ||
echo "FALSE" | ||
fi | ||
--- | ||
kind: TaskRun | ||
apiVersion: tekton.dev/v1beta1 | ||
metadata: | ||
name: write-array-tr | ||
spec: | ||
taskRef: | ||
name: write-array | ||
kind: task |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -141,17 +141,43 @@ type ArrayOrString struct { | |||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// UnmarshalJSON implements the json.Unmarshaller interface. | ||||||||||||||||||||||||||
func (arrayOrString *ArrayOrString) UnmarshalJSON(value []byte) error { | ||||||||||||||||||||||||||
switch value[0] { | ||||||||||||||||||||||||||
case '[': | ||||||||||||||||||||||||||
arrayOrString.Type = ParamTypeArray | ||||||||||||||||||||||||||
return json.Unmarshal(value, &arrayOrString.ArrayVal) | ||||||||||||||||||||||||||
case '{': | ||||||||||||||||||||||||||
arrayOrString.Type = ParamTypeObject | ||||||||||||||||||||||||||
return json.Unmarshal(value, &arrayOrString.ObjectVal) | ||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||
// ArrayOrString is used for Results Value as well, the results can be any kind of | ||||||||||||||||||||||||||
// data so we need to check if it is empty. | ||||||||||||||||||||||||||
if len(value) == 0 { | ||||||||||||||||||||||||||
arrayOrString.Type = ParamTypeString | ||||||||||||||||||||||||||
return json.Unmarshal(value, &arrayOrString.StringVal) | ||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this impact parameters with no values? is it covered by alternate validation to make sure a required parameter is specified - please confirm that the parameter validation has a test to check for no empty value for required params. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Could you point me to a case where parameters has no values?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So validation will check if the param is empty: pipeline/pkg/reconciler/taskrun/validate_resources.go Lines 110 to 121 in 7aef957
|
||||||||||||||||||||||||||
if value[0] == '[' { | ||||||||||||||||||||||||||
// We're trying to Unmarshal to []string, but for cases like []int or other types | ||||||||||||||||||||||||||
// of nested array which we don't support yet, we should continue and Unmarshal | ||||||||||||||||||||||||||
// it to String. If the Type being set doesn't match what it actually should be, | ||||||||||||||||||||||||||
// it will be captured by validation in reconciler. | ||||||||||||||||||||||||||
// if failed to unmarshal to array, we will convert the value to string and marshal it to string | ||||||||||||||||||||||||||
var a []string | ||||||||||||||||||||||||||
if err := json.Unmarshal(value, &a); err == nil { | ||||||||||||||||||||||||||
arrayOrString.Type = ParamTypeArray | ||||||||||||||||||||||||||
arrayOrString.ArrayVal = a | ||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized the errors didn't get thrown here. This should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I did this is because for case like nested lists or lists using not string element, the UnmarshalJson won't work. I agree this part of code is not writing very well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see. Then this should be fine. We will capture the type mismatch when you validate the results against PropertiesSpec. So it definitely worths adding more comments here and in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! Thanks for the suggestions! |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if value[0] == '{' { | ||||||||||||||||||||||||||
// if failed to unmarshal to map, we will convert the value to string and marshal it to string | ||||||||||||||||||||||||||
var m map[string]string | ||||||||||||||||||||||||||
if err := json.Unmarshal(value, &m); err == nil { | ||||||||||||||||||||||||||
arrayOrString.Type = ParamTypeObject | ||||||||||||||||||||||||||
arrayOrString.ObjectVal = m | ||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Yongxuanzhang marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// By default we unmarshal to string | ||||||||||||||||||||||||||
arrayOrString.Type = ParamTypeString | ||||||||||||||||||||||||||
if err := json.Unmarshal(value, &arrayOrString.StringVal); err == nil { | ||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
arrayOrString.StringVal = string(value) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function does look weird. Can we avoid using
Also, the unmarshalling does set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to use if, and the Yes, it is a bit weird that we need to set it to nil since we tried to unmarshal to array and it will create an empty array. It will be worth figuring out a better idea of this as you suggested |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// MarshalJSON implements the json.Marshaller interface. | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ type TaskRunResult struct { | |
Type ResultsType `json:"type,omitempty"` | ||
|
||
// Value the given value of the result | ||
Value string `json:"value"` | ||
Value ArrayOrString `json:"value"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a API change to me. It seems like we need to follow https://github.com/tektoncd/pipeline/blob/main/api_compatibility_policy.md#alpha-beta-and-ga for adding new feature and deprecation rules. Pinging @dibyom for confirmation. This is a field that users can specify. I think we should add a separate field for the new ArrayOrString and implement on the new field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a change, but it won't have effect on users or introduce breaking changes. And our TEP0076 requires us to change this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We came to a conclusion that this won't be breaking the API change policy. However, we definitely are blocked by the design about whether we want to have a brand new struct on this or just rename StringOrArray. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok but it doesn't block this PR right, I'm still using StringOrArray. We need to figure out that in Chuang's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah sure! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc:@ywluogg, I remember you send email to remind the changes, so we should send emails specifically about cli for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like api mismatch between the pipeline and cli, it should be fixed when we release this change and then bump the pipeline version for cli? Am I correct? @pritidesai There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /cc @tektoncd/cli-maintainers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think It will be fixed when CLI bumps the pipelines version (which will have these changes) and update their usage/implementation to change the type of the results from
|
||
} | ||
|
||
// ResultsType indicates the type of a result; | ||
|
@@ -54,7 +54,9 @@ type ResultsType string | |
// Valid ResultsType: | ||
const ( | ||
ResultsTypeString ResultsType = "string" | ||
ResultsTypeArray ResultsType = "array" | ||
ResultsTypeObject ResultsType = "object" | ||
) | ||
|
||
// AllResultsTypes can be used for ResultsTypes validation. | ||
var AllResultsTypes = []ResultsType{ResultsTypeString} | ||
var AllResultsTypes = []ResultsType{ResultsTypeString, ResultsTypeArray, ResultsTypeObject} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,22 +17,21 @@ import ( | |
"context" | ||
"fmt" | ||
|
||
"github.com/tektoncd/pipeline/pkg/apis/config" | ||
"knative.dev/pkg/apis" | ||
) | ||
|
||
// Validate implements apis.Validatable | ||
func (tr TaskResult) Validate(_ context.Context) *apis.FieldError { | ||
func (tr TaskResult) Validate(ctx context.Context) (errs *apis.FieldError) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ywluogg I made some new changes to this func, so we can fail fast if the alpha gate is not enabled |
||
if !resultNameFormatRegex.MatchString(tr.Name) { | ||
return apis.ErrInvalidKeyName(tr.Name, "name", fmt.Sprintf("Name must consist of alphanumeric characters, '-', '_', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my-name', or 'my_name', regex used for validation is '%s')", ResultNameFormat)) | ||
} | ||
// Validate the result type | ||
validType := false | ||
for _, allowedType := range AllResultsTypes { | ||
if tr.Type == allowedType { | ||
validType = true | ||
} | ||
// Array and Object is alpha feature | ||
if tr.Type == ResultsTypeArray || tr.Type == ResultsTypeObject { | ||
return errs.Also(ValidateEnabledAPIFields(ctx, "results type", config.AlphaAPIFields)) | ||
} | ||
if !validType { | ||
|
||
if tr.Type != ResultsTypeString { | ||
pritidesai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return apis.ErrInvalidValue(tr.Type, "type", fmt.Sprintf("type must be string")) | ||
} | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
In addition to the
Task
, please add an examplePipeline
. The results are more of a communication means between two tasks (or pipelineTasks). The example are great way to share the usage of the new features.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.
Hi @pritidesai, thanks for your comment!
In this PR array results passed between tasks are not implemented. This commit focuses on enabling emitting array results from entrypoint->controller.
I'm planning to add it (also array indexing) as well as examples in the followup PR.
Or should I add it in this PR? What do you think?
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.
Or maybe I should remove this example and add both examples for next pr?
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.
Thank you @Yongxuanzhang for the explanation.
You mean just the indexing is not supported in this PR or in general, the consumer will get an array result as a string value? I am trying to understand what happens when a consumer refers to such array result in a traditional way i.e.
$tasks.taskA.results.arrayResult
.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.
So with this PR, if referred
$tasks.taskA.results.arrayResult
in params and it is an array, the users will get a string of$tasks.taskA.results.arrayResult
not the array or the string format of the array.It is also something related to this issue.
This is a bit tricky part and it would be better to be addressed in a separate PR. Since
$tasks.taskA.results.arrayResult
or$tasks.taskA.results.arrayResult[*]
will be treated asstring
type inArrayOrString
.So this PR only enables the Task level results array, not sure if this is a real case.
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.
Oh I realized that the doc I added may be confusing as well, support emitting array results may let users think that they can pass array results between tasks within a pipeline, but it is not implemented in this PR.
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.
It might be or might not be, we do not advice such usage but its not restricted either.
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.
Should I fix the readme to clarify this? @pritidesai
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.
thank you @Yongxuanzhang I added comment ⬆️
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.
I have added more clarification in the doc