Fix json annotations in WhenExpressions #3389

jerop · 2020-10-14T22:05:17Z

Changes

When a pipeline with when expressions is created in v0.16.* then it is run in a pipelinerun in v0.17.0 or v0.17.1, a pipeline validation error about missing fields would be thrown -- as reported in #3382

That happened because json annotations were added to when expressions type in v0.17.0 so that the fields would have lowercase, such as in the code completion in tekton intellij plugin as described in redhat-developer/intellij-tekton#223

Without the json annotations, the fields were stored with first letters capitalized, that is Input, Operator and Values. With the json annotations, the fields were expected to be lowercase, that is input, operator and values, causing the missing fields error in pipeline validation

As such, users would have to reapply pipelines definitions created in previous versions to make them work in v0.17.0 or v0.17.1 -- to remove this requirement, we need to support both the uppercase and lowercase first letters for the annotations

Split up the when expressions example to separate pipelinerun and pipeline to make it easier to test a pipeline created in v0.16.3 and run with these changes:

pipeline: https://gist.github.com/jerop/dd2d64fbc6de5b353ea449c02a81b59e
pipelinerun: https://gist.github.com/jerop/13a58279cb5553c9b5d27f6735c23bbb

Fixes #3382

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

pipelines with when expressions created in v0.16.3 can be run

/kind cleanup
/cc @pritidesai @bobcatfish

pritidesai

thanks @jerop

pritidesai · 2020-10-14T22:45:43Z

/cc @vdemeester

jerop · 2020-10-15T12:53:50Z

/test pull-tekton-pipeline-go-coverage

vdemeester

So I wonder if we should "just" revert (and cherry-pick real quick to do a 0.17.2) or if we should go for a cleaner fix.

The cleaner fix would be to have Input (DeprecatedInput field in go) and input (Input field in go) and have some code to convert the Input into input.

I am ok with both, either we get that fix for "conversion" fix in 0.17.2, or in 0.18.0 (with a revert)

bobcatfish · 2020-10-15T14:50:22Z

@vdemeester I'm torn too and am actually really interested if you have a preference.

I like that the revert will "correct" the situation to some extent without the risk of us rushing in more extensive changes (also side note i suggested this to @jerop ) - but if anyone has used when expressions with 0.17.0 or 0.17.1, i think this will be a backwards incompatible change for them. i'm not certain, but i think going from storing the fields with lowercase (0.17.0 or 0.17.1) back to uppercase (this change) would be backwards incompatible? since probably less ppl are in this situation than the other, i think it's worth it?

I agree with you re. the cleaner fix, i think that's definitely the way to go, I guess it's a question of whether it's worth an interim release that "corrects" the backwards incompatible change in the meantime 🤔 🤔 🤔

dlorenc · 2020-10-19T12:59:41Z

/lgtm

jerop · 2020-10-19T13:48:24Z

~~/hold (pushing the cleaner fix shortly)~~

vdemeester · 2020-10-19T14:36:31Z

/hold

jerop · 2020-10-19T14:51:39Z

split up the when expressions example to separate pipelinerun and pipeline to make it easier to test pipeline created in v0.16.3 and run with these changes:

pipeline: https://gist.github.com/jerop/dd2d64fbc6de5b353ea449c02a81b59e
pipelinerun: https://gist.github.com/jerop/13a58279cb5553c9b5d27f6735c23bbb

tekton-robot · 2020-10-19T17:49:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai, sbwsg

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:

~~OWNERS~~ [pritidesai,sbwsg]

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

tekton-robot · 2020-10-19T18:21:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/when_validation.go	100.0%	91.9%	-8.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.9%	92.6%	0.7

When a pipeline with when expressions is created in v0.16.* then it is run in a pipelinerun in v0.17.0 or v0.17.1, a pipeline validation error about missing fields would be thrown -- as reported in tektoncd#3382 That happened because json annotations were added to when expressions type in v0.17.0 so that the fields would have lowercase, such as in the code completion in tekton intellij plugin as described in redhat-developer/intellij-tekton#223 Without the json annotations, the fields were stored with first letters capitalized, that is Input, Operator and Values. With the json annotations, the fields were expected to be lowercase, that is input, operator and values, causing the missing fields error in pipeline validation As such, users would have to reapply pipelines definitions created in previous versions to make them work in v0.17.0 or v0.17.1 -- to remove this requirement, we need to support both the uppercase and lowercase first letters for the annotations Fixes tektoncd#3382

jerop · 2020-10-19T19:06:09Z

/retest

jerop · 2020-10-19T19:20:23Z

/test pull-tekton-pipeline-go-coverage

pritidesai · 2020-10-19T19:41:02Z

@bobcatfish @vdemeester folks have started migrating their pipelines to use when expressions with 0.16, migrating to 0.17 without this fix would break them. Thanks @jerop for implementing the cleaner fix.

vdemeester

/lgtm

vdemeester · 2020-10-21T08:38:45Z

/hold cancel

We fixed the json annotations and deprecated the PascalCase fields in tektoncd#3389 -- this change includes these deprecations in the table for tracking

We fixed the json annotations and deprecated the PascalCase fields in #3389 -- this change includes these deprecations in the table for tracking

The fields in `WhenExpressions` were missing json annotations in tektoncd#3135 so they were PascalCase - relased in v0.16 The json annotations were added and supported both PascalCase and lowercase fields for backwards compatibility in tektoncd#3291 and tektoncd#3389 - released in v0.17 The PascalCase fields were deprecated in v0.17, with an earliest date of removal of Jan 07 2021 - https://github.com/tektoncd/pipeline/blob/v0.23.0/docs/deprecations.md This change removes the deprecated PascalCase fields in `WhenExpressions`

This change removes the deprecated PascalCase fields in `WhenExpressions` The fields in `WhenExpressions` were missing json annotations in tektoncd#3135 so they were PascalCase - released in v0.16 The json annotations were added and supported both PascalCase and lowercase fields for backwards compatibility in tektoncd#3291 and tektoncd#3389 - released in v0.17 The PascalCase fields were deprecated in v0.17, with an earliest date of removal of Jan 07 2021 - https://github.com/tektoncd/pipeline/blob/v0.23.0/docs/deprecations.md

This change removes the deprecated PascalCase fields in `WhenExpressions` The fields in `WhenExpressions` were missing json annotations in #3135 so they were PascalCase - released in v0.16 The json annotations were added and supported both PascalCase and lowercase fields for backwards compatibility in #3291 and #3389 - released in v0.17 The PascalCase fields were deprecated in v0.17, with an earliest date of removal of Jan 07 2021 - https://github.com/tektoncd/pipeline/blob/v0.23.0/docs/deprecations.md

This change removes the deprecated PascalCase fields in `WhenExpressions` The fields in `WhenExpressions` were missing json annotations in tektoncd#3135 so they were PascalCase - released in v0.16 The json annotations were added and supported both PascalCase and lowercase fields for backwards compatibility in tektoncd#3291 and tektoncd#3389 - released in v0.17 The PascalCase fields were deprecated in v0.17, with an earliest date of removal of Jan 07 2021 - https://github.com/tektoncd/pipeline/blob/v0.23.0/docs/deprecations.md

This change removes the deprecated PascalCase fields in `WhenExpressions` The fields in `WhenExpressions` were missing json annotations in #3135 so they were PascalCase - released in v0.16 The json annotations were added and supported both PascalCase and lowercase fields for backwards compatibility in #3291 and #3389 - released in v0.17 The PascalCase fields were deprecated in v0.17, with an earliest date of removal of Jan 07 2021 - https://github.com/tektoncd/pipeline/blob/v0.23.0/docs/deprecations.md

tekton-robot requested review from bobcatfish and pritidesai October 14, 2020 22:05

pritidesai approved these changes Oct 14, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2020

pritidesai added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Oct 14, 2020

tekton-robot requested a review from vdemeester October 14, 2020 22:45

vdemeester reviewed Oct 15, 2020

View reviewed changes

tekton-robot assigned dlorenc Oct 19, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2020

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2020

vdemeester added this to the Pipelines v0.17 milestone Oct 19, 2020

jerop force-pushed the revert-3291-json-annotation branch from b01c381 to 84734ee Compare October 19, 2020 14:40

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 19, 2020

jerop changed the title ~~Revert "Add missing json annotations"~~ Fix json annotations in WhenExpressions Oct 19, 2020

jerop force-pushed the revert-3291-json-annotation branch from 84734ee to 4033ab8 Compare October 19, 2020 17:34

ghost approved these changes Oct 19, 2020

View reviewed changes

jerop force-pushed the revert-3291-json-annotation branch from 4033ab8 to 570cc86 Compare October 19, 2020 18:17

jerop force-pushed the revert-3291-json-annotation branch from 570cc86 to 4f35ed1 Compare October 19, 2020 18:30

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2020

vdemeester reviewed Oct 21, 2020

View reviewed changes

tekton-robot assigned vdemeester Oct 21, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2020

tekton-robot merged commit 4edcbc1 into tektoncd:master Oct 21, 2020

vdemeester removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Oct 21, 2020

jerop mentioned this pull request Nov 30, 2020

Add deprecated PascalCase fields in WhenExpressions to deprecated table #3570

Merged

4 tasks

jerop mentioned this pull request May 7, 2021

Remove deprecated PascalCase fields in WhenExpressions #3923

Merged

5 tasks

jerop mentioned this pull request Jun 7, 2021

Fix Workspaces in Sidecar to be serialized as workspaces not Workspaces #3966

Merged

5 tasks

jerop deleted the revert-3291-json-annotation branch June 11, 2022 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix json annotations in WhenExpressions #3389

Fix json annotations in WhenExpressions #3389

jerop commented Oct 14, 2020 •

edited

Loading

pritidesai left a comment

pritidesai commented Oct 14, 2020

jerop commented Oct 15, 2020

vdemeester left a comment

bobcatfish commented Oct 15, 2020

dlorenc commented Oct 19, 2020

jerop commented Oct 19, 2020 •

edited

Loading

vdemeester commented Oct 19, 2020

jerop commented Oct 19, 2020 •

edited

Loading

tekton-robot commented Oct 19, 2020

tekton-robot commented Oct 19, 2020

jerop commented Oct 19, 2020

jerop commented Oct 19, 2020

pritidesai commented Oct 19, 2020

vdemeester left a comment

vdemeester commented Oct 21, 2020

Fix json annotations in WhenExpressions #3389

Fix json annotations in WhenExpressions #3389

Conversation

jerop commented Oct 14, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

pritidesai left a comment

Choose a reason for hiding this comment

pritidesai commented Oct 14, 2020

jerop commented Oct 15, 2020

vdemeester left a comment

Choose a reason for hiding this comment

bobcatfish commented Oct 15, 2020

dlorenc commented Oct 19, 2020

jerop commented Oct 19, 2020 • edited Loading

vdemeester commented Oct 19, 2020

jerop commented Oct 19, 2020 • edited Loading

tekton-robot commented Oct 19, 2020

tekton-robot commented Oct 19, 2020

jerop commented Oct 19, 2020

jerop commented Oct 19, 2020

pritidesai commented Oct 19, 2020

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester commented Oct 21, 2020

jerop commented Oct 14, 2020 •

edited

Loading

jerop commented Oct 19, 2020 •

edited

Loading

jerop commented Oct 19, 2020 •

edited

Loading