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.

Sign up for GitHub

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

Jump to bottom

Allow . in param/result names via subscript. #4215

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Allow . in param/result names via subscript. #4215

merged 2 commits into from
Sep 15, 2021

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented Sep 5, 2021 •
edited
Loading

This change allow folks to use . in parameter names (e.g. dev.mattmoor.my-param), and reference them via the subscript operator (e.g. params["dev.mattmoor.my-param"]) to avoid ambiguity introduced by the mixing of .s.

TEP: tektoncd/community#503

/kind enhancement
/hold

Holding until the TEP is merged.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Parameter and result names may now contain `.` and be referenced via the subscript operator (e.g. `$(params["foo.bar"])`

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 5, 2021
@tekton-robot tekton-robot requested review from dlorenc and a user September 5, 2021 21:40
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 5, 2021
@mattmoor
Copy link
Member Author

mattmoor commented Sep 5, 2021

I'm sure there are some docs changes needed for this, although I'm not sure where those docs are, so if folks could provide pointers to any docs they think should be updated to reflect this change I'd appreciate it 🙏

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 5, 2021
@tekton-robot
Copy link
Collaborator

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/reconciler/taskrun/resources/apply.go 99.2% 99.2% 0.0

@mattmoor
Copy link
Member Author

mattmoor commented Sep 5, 2021

I'm going to try and expand this with e2e test coverage as well, so not checking the test box yet.

@mattmoor
Allow . in param/result names via subscript.
3ab7f06 
This change allow folks to use `.` in parameter names (e.g. `dev.mattmoor.my-param`), and reference them via the subscript operator (e.g. `params["dev.mattmoor.my-param"]`) to avoid ambiguity introduced by the mixing of `.`s.

TEP: tektoncd/community#503
@tekton-robot
Copy link
Collaborator

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/reconciler/taskrun/resources/apply.go 99.2% 99.2% 0.0

@mattmoor
Copy link
Member Author

mattmoor commented Sep 5, 2021

Only failure seems to be outside of what I've modified, so let's see if it repro's.

/retest

@mattmoor
Copy link
Member Author

mattmoor commented Sep 5, 2021

I think docs/tasks.md may be the place to update for the docs ✅ , looking at that now.

@tekton-robot
Copy link
Collaborator

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/reconciler/taskrun/resources/apply.go 99.2% 99.2% 0.0

@mattmoor
Copy link
Member Author

mattmoor commented Sep 5, 2021

Ok, I updated docs/tasks.md and also found docs/variables.md and added a couple rows to the pertinent tables with the new syntax. The prior commit also tweaked a few of the e2e tests to exercise the subscript syntax, so I've checked off "docs" and "e2e". I'm ofc happy to expand both of these if folks have specific pointers to things I missed.

For the commit message, I've left this unchecked as I am somewhat reluctant to fork the TEP PR as the source of truth (esp. before it is merged), but if folks have a good example, I'd be happy to circle back and update things to follow a strong precedent.

@mattmoor
Copy link
Member Author

mattmoor commented Sep 6, 2021

/unhold

The TEP has merged

@vdemeester
Copy link
Member

/hold cancel
Maybe ? 😛

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2021
@mattmoor
Copy link
Member Author

mattmoor commented Sep 6, 2021

/unhold works for Knative prow, so either we have some snowflake or y'all are on a really old version 😅

Either way, thanks!

@vdemeester
Copy link
Member

/unhold works for Knative prow, so either we have some snowflake or y'all are on a really old version

Either way, thanks!

Really old version it is 😉 😛 😅

@dlorenc
Copy link
Contributor

dlorenc commented Sep 7, 2021

/lgtm

nice!

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2021
vdemeester
vdemeester approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold
/cc @afrittoli @bobcatfish

pkg/reconciler/taskrun/resources/apply.go
Comment on lines +47 to +48
// FIXME(vdemeester) Remove that with deprecating v1beta1
"inputs.params.%s",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I wrote that FIXME ? I think I meant "with deprecating v1alpha1", maybe ? 🤔 👴🏼

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just moved it 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.. I saw that 😝 I am confused by my own comment 😹 damn…

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2021
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2021
@vdemeester
Copy link
Member

(holding just a day or two or less to allow core team to review 👼🏼 )

@dlorenc
Copy link
Contributor

dlorenc commented Sep 10, 2021

Are we still waiting on anything here?

@vdemeester
Copy link
Member

@dlorenc let's validate this in the upcoming API WG (today) 👼🏼

@mattmoor
Copy link
Member Author

@vdemeester Just seeing this, but don't think it was discussed... 😅

@vdemeester
Copy link
Member

@vdemeester Just seeing this, but don't think it was discussed...

That's a good point 😅
@tektoncd/core-maintainers last call for review, I'll check tomorrow morning before the release 😝

@mattmoor
Copy link
Member Author

Just popped open my pending PR list and saw this (I'd forgotten about it 🤣 ), should we /hold cancel it?

@vdemeester
Copy link
Member

/hold cancel
Indeed 😛

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2021
@tekton-robot tekton-robot merged commit 4642f04 into tektoncd:main Sep 15, 2021
@bobcatfish
Copy link
Collaborator

Late to the party but I'm excited to see this going in! Next release is gonna be a good one :D

@bobcatfish bobcatfish mentioned this pull request Sep 15, 2021
Merged
@bobcatfish
Copy link
Collaborator

That being said, one more process nit: technically the TEP should be "implementable" before the implementation gets merged (another opportunity for some automation to enforce?) - I added a comment to the TEP pr also https://github.com/tektoncd/community/pull/503/files#r709442151

@mattmoor mattmoor deleted the allow-dots-via-subscripts branch September 15, 2021 18:22
@mattmoor
Copy link
Member Author

I'm curious if there are examples of TEPs that have been merged as "proposed" and never been made "implementable"? I'm sort of curious why this extra hop is in there, and whether it's been useful historically?

Happy to discuss elsewhere as this is sort of an odd place to have that discussion 🤣

@bobcatfish
Copy link
Collaborator

@mattmoor wouldn't be surprised - the process is documented at https://github.com/tektoncd/community/blob/main/teps/0001-tekton-enhancement-proposal-process.md#tep-workflow - looking at some of the history there might help; a great place to discuss changes would be the API working group and/or an issue/pr over there

@skaegi
Copy link
Contributor

skaegi commented Sep 27, 2021 •
edited
Loading

I know this PR is merged already, but I think it would make a lot of sense to support single quotes in addition to double quotes. This both matches how Kubernetes references info with the downwards api and also how bracket notation is typically used.

In terms of implementation I think this is just a matter of adding "params['%s']", to the patterns arrays as well as tests. I can create a PR when I get a moment.

@skaegi skaegi mentioned this pull request Sep 30, 2021
Merged
5 tasks
tekton-robot pushed a commit that referenced this pull request Oct 5, 2021
@skaegi @tekton-robot
Support for single-quote bracket notation for params
72388ca 
This change adds single-quote bracket notation to the work done in #4215. This is consistent with how referencing is done else in K8s in the downwards api.

The original patch used the name subscript notation to describe this however the standard name for this approach is bracket notation and updates the doc accordingly.
chenbh pushed a commit to chenbh/pipeline that referenced this pull request Oct 27, 2021
@skaegi @chenbh
Support for single-quote bracket notation for params
dd4591e 
This change adds single-quote bracket notation to the work done in tektoncd#4215. This is consistent with how referencing is done else in K8s in the downwards api.

The original patch used the name subscript notation to describe this however the standard name for this approach is bracket notation and updates the doc accordingly.
@tlawrie tlawrie mentioned this pull request May 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@dlorenc dlorenc Awaiting requested review from dlorenc

@afrittoli afrittoli Awaiting requested review from afrittoli

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

Assignees

@dlorenc dlorenc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mattmoor @tekton-robot @vdemeester @dlorenc @bobcatfish @skaegi