[TEP-0143] Concise Parameters and Results - proposed

[Yongxuanzhang]

This commit adds the proposal of concise parameters and results.

Signed-off-by: Yongxuan Zhang [email protected]

[Yongxuanzhang]

Hi @tektoncd/core-maintainers, ptal at this proposal, looking forward to your thoughts! Thanks!

[Yongxuanzhang]

cc @khrm @pritidesai
would you be interested in reviewing this?

[chitrangpatel]

/kind tep

[dibyom]

/assign @pritidesai
/assign @dibyom

[vdemeester]

I like the idea of that proposal (@abayer knows how often I bugged him about this param/result syntax 😛), but I don't see a way to have it in v1. I would see this as the basis of a v2 API work.

[vdemeester]

"Users need to repeatedly write name and value […]" applies on PipelineRun am I right ?

[Yongxuanzhang]

Yes! The proposal includs pipelinerun, taskrun, and pipeline, task

[vdemeester]

We used to have inputs as well, so I would be very caution re-introducing it.

[Yongxuanzhang]

I think we will introduce them back in #1044, cc @jerop @afrittoli to confirm.
The main reason is that we can not change the current params and results, and we need to put them under a new api field, and tep-0139 will introduce artifacts with input and output, so it makes sense to add params and results there

[vdemeester]

I would definitely want to go that way for that proposal (aka going towards a v2… API). If we do not, we are going to make the v1 API more convoluted as we go. And we cannot ever remove fields from v1 so.. having 2 ways to provide the same information on a API is generally not really ideal, and will be very confusing and very tricky to achieve implementation wise (what do we do if both are used, …)

[Yongxuanzhang]

Yes that's a very good point and we have also discussed it as well!
From the implementation wise, 1) this will be an alpha feature gated by dedicated feature flag, this can make sure that users won't accidentally define both. 2) we will add validation to the webhook to make sure that only one of them is defined.

I agree that it is not ideal that users can define the same thing in 2 places, the way we mitigate it is to use feature flag, documentation.

And the main reason we propose to add in v1 is that v1 users can try out this new syntax and provide early feedback. Another reason is introducing v2alpha1 will introduce more maintaining efforts to tekton pipeline (not sure if this is a valid concern).

I think we're always open change the proposal to what's best for tekton 😄 .

[Yongxuanzhang]

Hi @vdemeester @afrittoli, just want to learn more about your thoughts on getting early feedback from users by adding them in v1 api.
From the implementation perspective, if we add v2alpha1, we will need more efforts, e.g.:

1. we need to consider conversion among v2alpha1, v1, v1beta1.
2. developers making api changes to v1 will update v2alpha1.

So I don't think this is a good timing to introduce v2alpha1 considering that we just have v1 this year, and there are not many features we can introduce in v2alpha1.

If you think this proposal is not a good fit to v1(duplicate with current params and results). We may just change this proposal to add it to v2 in the future and don't work on it now.

[QuanZhang-William]

+1 this is not ideal UX in v1. To me, the most valuable part to introduce this feature now but not in v2 is that we can collect some early feedback/data points which can help us make the right decision when moving to v2.

I think gating it under a dedicated feature flag can mitigate the confusion to user. However, we may need to be more careful to promote this feature to beta or stable where the feature will be opted in by default (as part of the reason to support the feature in v1 is to collect early feedback).

We also need clear documentation to explain the context.

[vdemeester]

Having 2 different ways to solve the same problems in the same API is, in my opinion, also not necessarily a good UX.
Looking back, we even moved from inputs.params to params while bumping API version (v1alpha1 to v1beta1) even though we could have done breaking changes in v1alpha1.

The proposal of adding this to v1, and in general to add a lot of new fields in v1 seems like we don't treat v1 as stable, or as complete enough to be considered as is. This is fine, but v1 is there, and is there for a while, and is considered stable from our user's perspective. We have to be very careful when adding new fields to v1.

Any field we add to v1 (even behind a feature-flag) are there to stay (they can't be removed). Any field we add to v1 need to be documented (and would automatically be if we fix tektoncd/pipeline#1461 (and tektoncd/pipeline#3416 at the same time) — and for each "alpha" or experimental ones, we need to make it show as it in the doc, etc..

Other question arrives : kubectl (and Kubernetes in general) is merging objects with kubectl apply for example. This lead to problems between v1alpha1 and v1beta1 as it was merging inputs.params with params. Having both on the same object would also be an issue — I created my object with params, I now want to try that new consice thing, I switch to inputs.params and I apply, it breaks. But because it's on v1, this issue could never be fixed for the v1 API as we cannot remove fields — we would be stuck with this issue until v2 (the stable one) and v1 gone and out of support (which would be.. in a really long time, years and years from now).

Having a v2alpha1 or a feature-flag is similar in that they require the user to do something. But one is way more explicit in its "experimental" nature, as you are using v2alphaX API, so you know you shouldn't use it in production, you know it could break tomorrow.

From the implementation perspective, if we add v2alpha1, we will need more efforts, e.g.:

For me, this is not a concern. The aim is end-user consumption, happyness, etc. If we need to do more effort to make a better user experience, I'll choose this over and over.

• we need to consider conversion among v2alpha1, v1, v1beta1.
• developers making api changes to v1 will update v2alpha1.

That's the thing about v2alpha1, it could be "incomplete" in way, as it's alpha, it's there for exploration.

[Yongxuanzhang]

Thanks!

Having 2 different ways to solve the same problems in the same API is not a good UX.

Yes I agree, even if it is gated by a feature flag, users may get confused.

we need to be very careful to add new api into v1.

Yes I agree. Trusted artifacts is proposing to add inputs.artifacts and outputs.artifacts, so for tep-0143 we just need to add params and results to them.

Having both on the same object would also be an issue ...

I think this can be avoided by how we implement this? My current PoC is using mutation webhook to populate current params with the inputs.params, and we don't need to change the reconciler code. So if there is existing pipelinerun with params, the new object would just have both inputs.params and paramsand won't break.

So I think "having 2 different ways of defining the same thing in api" is a valid reason that we should not add it to v1

[afrittoli]

Having 2 different ways to solve the same problems in the same API is not a good UX.

Yes I agree, even if it is gated by a feature flag, users may get confused.

Indeed, so maybe we shouldn't do it?

we need to be very careful to add new api into v1.

Yes I agree. Trusted artifacts is proposing to add inputs.artifacts and outputs.artifacts, so for tep-0143 we just need to add params and results to them.

That's not the API I proposed for trusted artifacts.
It was later proposed to adopt inputs.artifacts and outputs.artifacts to match this proposal, not the other way around.
I'm still unsure whether we need the extra level of indentation with inputs and outputs. Do you have any more insights on the advantages of that?

I would argue that a v1alpha1 might be more easily adopted by users, because unless operators explicitly disable v1alpha1, users can experiment with it. Feature flags instead are controller by the operator.

Using a different API version would mean that users could try and query existing resources via the v1alpha1 endpoint, to see how existing resources would look like in v1alpha1 format.

[Yongxuanzhang]

Thanks! @afrittoli are you suggesting v1alpha1 or v2alpha1?

It was later proposed to adopt inputs.artifacts and outputs.artifacts to match this proposal, not the other way around.
I'm still unsure whether we need the extra level of indentation with inputs and outputs. Do you have any more insights on the advantages of that?

Oh if so, I think we can just don't use inputs and outputs. I will update this PR to propose the change in a new api version

[khrm]

We lose Type safety with this TEP.

[Yongxuanzhang]

We lose Type safety with this TEP.

Thanks! We have discussed this a bit in the thread of this doc. And we have also mentioned that this is a drawback in this proposal, I don't think there is a way to really solve it (e.g. we cannot really tell users that they have defined duplicate params with the map syntax)

We can provide explicit documentation, yaml syntax check tools to mitigate this issue and I believe this is the trade-off we can make.

Or there might be more severe type safety issues we miss? Please let us know, and if the proposal cannot address it, I think we can seek other alternatives.

[khrm]

@Yongxuanzhang Yes, I didn't get time to discuss that further.

[khrm]

This is fine and safe. It will be validated easily during the creation. It will be put to

map[string]struct.

[khrm]

This is unsafe. I assume it will be decoded/unmarshal to:

map[string]interface{}

? Isn't it so?

[khrm]

What happens is that we might need to have some reserved keywords for the string key.
What happens if someone mistakenly indents serviceAccountName at the same level? Or some other top elements.

The present API won't let you create these.

[jkandasa]

but the all these key: value, fields defined under params right?
serviceAccountName comes under params?

[khrm]

serviceAccountName doesn't come under params. It's on a different level but here we don't know whether key serviceAccountName is some params or spec.serviceAccountName . So we can't validate. We will need to reserve this.

[khrm]

On a side note, we shouldn't think of this from trying to solve validation via Go code. Is it easier to validate in 1.26+ k8s using CEL Validating admission policies?

[Yongxuanzhang]

This is unsafe. I assume it will be decoded/unmarshal to:

map[string]interface{}

? Isn't it so?

We will still use this function to unmarshal it, it will be map[string]ParamValue, so it is not interface{}

[Yongxuanzhang]

What happens is that we might need to have some reserved keywords for the string key. What happens if someone mistakenly indents serviceAccountName at the same level? Or some other top elements.

The present API won't let you create these.

Are you saying that the proposal won't work with reserved param? Let's say is a "paramA" is reserved, we can just add function to validation webhook to check if the map[string]ParamValue has a key of "paramA"?

[afrittoli]

@khrm @Yongxuanzhang I'm not 100% I got it right, this is my understanding of the issue.
Current TaskRun spec:

spec:
  params:
  - name: example
    value: foo
  serviceAccountName: default

New TaskRun spec:

spec:
  input:
    params:
      example: foo
  serviceAccountName: default

With the new spec, if serviceAccountName was not indented properly, you could have:

spec:
  input:
    params:
      example: foo
      serviceAccountName: default

Which would not fail validation if the Task in question had a parameter called serviceAccountName.
This could not happen with the current API. This could be prevented in the new syntax by forbidding serviceAccountName as a parameter name.

[Yongxuanzhang]

Thanks for this, it's a very interesting idea!

If you have context on why the current syntax was chosen it would help better understanding what we might be losing with the change.

@afrittoli Thanks!
The issue author suggests using map syntax because it will reduce the verbosity, which is the same motivation for our proposal.
One reason not using map is to avoid confusions for novice users, pointed out in this comment.

This could be a trade off we make and I think we need to collect users feedback as well. We cannot propose something we think good for users but actually causes confusions. So I will change this proposal to proposed status and this could be something we consider for v2. At that time we will collect users' feedback and see if this is a good change?

[Yongxuanzhang]

Hi @vdemeester, @afrittoli, @pritidesai I have updated the proposal to proposed status, and change it to suggest adding in v2alpha1. Please take a look. Thanks! 🙏

[afrittoli]

About the removal of type, I guess that's because it can be inferred from the format of the result?

[afrittoli]

Thanks for the update, it looks good to me.
I still have a couple of comments, but nothing blocking the "proposed" state for me.
/approve

[pritidesai]

/assign @khrm

[jerop]

@dibyom @khrm @pritidesai can we please merge this as proposed? we will continue to discuss as we work towards making this proposal implementable

[dibyom]

Merging as proposed sounds fine to me
/approve

[khrm]

/approve

Yes.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dibyom, khrm, vdemeester

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:

• teps/OWNERS [afrittoli,dibyom,khrm,vdemeester]

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

[jerop]

Thank you!

/lgtm