Skip to content
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

Params copied in all steps in saved pipeline in alpha mode on Openshift with tekton 0.29+ #4388

Closed
lstocchi opened this issue Nov 19, 2021 · 9 comments · Fixed by #4511
Closed
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.

Comments

@lstocchi
Copy link

Expected Behavior

Save my pipeline as it is

Actual Behavior

It's like params are being copied in all steps.

original pipeline -> https://gist.github.com/lstocchi/f7dc7aa051489001d76456493cdc6b73
saved pipeline (look at params in each step) -> https://gist.github.com/lstocchi/e9e7090c9eac6d329fcf80405f10f09e

Steps to Reproduce the Problem

  1. have a cluster with tekton v. 0.29 +
  2. enable alpha features enable-api-fields: alpha
  3. try to save a pipeline on cluster

Additional Info

  • Kubernetes version: Tried with two openshift clusters

    Output of kubectl version:

Client Version: 4.8.5
Server Version: 4.9.5
Kubernetes Version: v1.22.0-rc.0+a44d0f0

and

Client Version: 4.8.5
Server Version: 4.10.0-0.nightly-2021-11-15-034648
Kubernetes Version: v1.22.1+f773b8b

  • Tekton Pipeline version:
Client version: 0.20.0
Pipeline version: v0.30.0
Triggers version: v0.17.1

and

Client version: 0.20.0
Pipeline version: v0.29.0
Triggers version: v0.17.1

It works fine with 0.28.0.

@lstocchi lstocchi added the kind/bug Categorizes issue or PR as related to a bug. label Nov 19, 2021
@lstocchi lstocchi changed the title Error when saving pipeline in alpha mode on Openshift with tekton 0.29+ Params copied in all steps in saved pipeline in alpha mode on Openshift with tekton 0.29+ Nov 19, 2021
@jerop
Copy link
Member

jerop commented Nov 22, 2021

@wlynch I believe this is related to implicit parameters

@wlynch
Copy link
Member

wlynch commented Nov 22, 2021

/assign wlynch

@wlynch
Copy link
Member

wlynch commented Nov 24, 2021

Hi - Thanks for the feedback!

This is intentional behavior, but by no means final (which is why it's behind the alpha flag). https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md is the proposal describing the feature. The parameter copying was intended to make it clear how the params are made available to the underlying steps. Documentation can be found here: https://github.com/tektoncd/pipeline/blob/main/docs/pipelineruns.md#implicit-parameters

This is good feedback though. Few questions:

  • Did the extra params cause any incorrect behavior? (i.e. used parameter values didn't get set correctly, size limits, etc).
  • What are your thoughts on the same implicit param behavior, but without the param copying?

This is an area I'm actively looking to improve, so would love to discuss this more!

@lstocchi
Copy link
Author

Hi @wlynch,

I wasn't aware of this feature, sorry. I works on the IJ tekton plugin and we usually don't cover alpha stuff but we made an exception for the taskrun debug which, from an IDE perspective, is a must-have. This is how I faced this behavior.

In my case, the pipeline couldn't get started because two params with the same name were in the same task.
revision line 45 -> https://gist.github.com/lstocchi/e9e7090c9eac6d329fcf80405f10f09e#file-gistfile1-txt-L45
REVISION line 49 -> https://gist.github.com/lstocchi/e9e7090c9eac6d329fcf80405f10f09e#file-gistfile1-txt-L49
so during start execution an error appeared saying there were 2 params with the same name.

By reading the documentation and looking at the motivation this feature makes sense. I was a bit scared because I didn't expect to see params i didn't inserted in my pipeline and i thought "damn it's our plugin fault??". But if you are aware of it, the only drawback is that the pipeline gets longer. Maybe adding a flag to disable implicit params would make sense. Expert users could prefer to rely only on what they write. Just my 2 cent.

Thanks!!

@ghost
Copy link

ghost commented Jan 13, 2022

Just to add another anecdotal here - we experienced some surprising behaviour in relation to this change during the release of 0.32 of Tekton Pipelines. One of our Pipelines relied on a default param value in a referenced Task, which was then unexpectedly overridden by the implicit param defined in the Pipeline.

It's tricky to describe in words, here's a link to the PipelineTask as we had it before we noticed this behaviour and here's a link to the PipelineTask after we corrected for it.

^ In the first link we don't pass the platforms param because we're relying on the default defined in the referenced Task. With implicit params that platforms param gets overridden with the value in the platforms param defined for the Pipeline. The reason for the difference in the values is a bit nuanced but regardless it looks like this might be accidentally backwards incompatible?

edit: tried to disambiguate that the default was set in the referenced task.

@jerop jerop added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jan 13, 2022
@jerop jerop added this to the Pipelines v0.32 milestone Jan 13, 2022
@wlynch
Copy link
Member

wlynch commented Jan 13, 2022

/remove-label priority/critical-urgent

@jerop There are multiple issues being described in this issue. The ref behavior bug we're want to fix ASAP will be tracked in #4483

@wlynch
Copy link
Member

wlynch commented Jan 24, 2022

Based on this issue + additional feedback from @bobcatfish and @jerop, we're going to go ahead and remove the implicit behavior for Pipeline objects in #4511 for now. Plan is to roll this back and wait for user feedback / more discussion to figure out if this is something we want to support for Pipelines (as opposed to just PipelineRun/TaskRuns) in the future.

@sushilHpal
Copy link

sushilHpal commented Feb 15, 2022

For 0.32.1 release of Tekton, which Kubernetes cluster version we need to have? can any one please response on the same.

@jerop
Copy link
Member

jerop commented Apr 7, 2022

For 0.32.1 release of Tekton, which Kubernetes cluster version we need to have? can any one please response on the same.

@sushilHpal for 0.32.1, you need Kubernetes version 1.20 or later

Required Kubernetes Versions:

  • Starting from the v0.24.x release of Tekton: Kubernetes version 1.18 or later
  • Starting from the v0.27.x release of Tekton: Kubernetes version 1.19 or later
  • Starting from the v0.30.x release of Tekton: Kubernetes version 1.20 or later
  • Starting from the v0.33.x release of Tekton: Kubernetes version 1.21 or later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants