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

add feature gate check for param array indexing #6120

Closed
wants to merge 2 commits into from

Conversation

Yongxuanzhang
Copy link
Member

@Yongxuanzhang Yongxuanzhang commented Feb 7, 2023

Changes

Before this commit, if alpha or beta feature gate is not enabled, the array indexing params will not be added to string replacements, thus will lead to non-existent variable error. This is confusing to users and doesn't provide correct guidance. This commit adds the check to the array indexing validation.

/kind bug

fixes #6102

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

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

  • Has Docs included if any changes are user facing
  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Add feature gate check for param array indexing

@tekton-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2023
@Yongxuanzhang
Copy link
Member Author

/test all

@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/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.7% -9.9

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.7% -9.9

@Yongxuanzhang Yongxuanzhang changed the title add feature gate check for param array indexing add alpha feature gate check for param array indexing Feb 8, 2023
@Yongxuanzhang
Copy link
Member Author

/test all

@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/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.5% -10.1

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.5% -10.1

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.5% -10.1

@Yongxuanzhang Yongxuanzhang marked this pull request as ready for review February 8, 2023 16:28
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2023
@Yongxuanzhang
Copy link
Member Author

hope this can fix #6102

@Yongxuanzhang
Copy link
Member Author

Yongxuanzhang commented Feb 8, 2023

/assign @pritidesai
not sure if there are better ways to do this, I'm looking forward to any suggestions!

@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/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.5% -10.1

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 85.7% -10.5
pkg/reconciler/taskrun/validate_resources.go 98.5% 88.5% -10.1

@Yongxuanzhang
Copy link
Member Author

If this approach is ok, I can add more cases to test err check to improve test cov

@Yongxuanzhang
Copy link
Member Author

Yongxuanzhang commented Feb 14, 2023

Thanks @Yongxuanzhang for this PR. It seems to me that this PR and #6103 have a dependency since this adds a new alpha flag related to indexing for array params and @pritidesai's PR promotes indexing for array parameters to beta. If my understanding is correct, we need to agree on which PR should be merged first, and updated the other one accordingly.

Yes, thanks! @afrittoli. I lean to merge @pritidesai's #6103 first. I will hold my PR and update this PR to use both alpha&beta check once it is merged

@Yongxuanzhang
Copy link
Member Author

/hold

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 14, 2023
@JeromeJu
Copy link
Member

Wondering if alpha feature gate checking would need to be in the release note? Or this is something that shall already been acknowledged before 🤔

@Yongxuanzhang
Copy link
Member Author

Wondering if alpha feature gate checking would need to be in the release note? Or this is something that shall already been acknowledged before 🤔

Sounds good, will add after I rebase

Before this commit, if alpha feature gate is not enabled, the array
indexing params will not be added to string replacements, thus will lead
to non-existent variable error. This is confusing to users and doesn't
provide correct guidance. This commit adds the check to the array
indexing validation.

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
@Yongxuanzhang Yongxuanzhang changed the title add alpha feature gate check for param array indexing add feature gate check for param array indexing Feb 15, 2023
@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesnt merit a release note. labels Feb 15, 2023
@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/pipelinerun/pipelinerun.go 87.1% 87.4% 0.3
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 86.2% -9.9
pkg/reconciler/taskrun/validate_resources.go 98.5% 87.0% -11.5

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/pipelinerun.go 87.1% 87.4% 0.3
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 86.2% -9.9
pkg/reconciler/taskrun/validate_resources.go 98.5% 87.0% -11.5

@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/pipelinerun/pipelinerun.go 87.1% 87.4% 0.3
pkg/reconciler/pipelinerun/resources/validate_params.go 96.2% 97.4% 1.3
pkg/reconciler/taskrun/validate_resources.go 98.5% 98.5% 0.0

@tekton-robot
Copy link
Collaborator

@Yongxuanzhang: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-tekton-pipeline-build-tests 936483d link true /test pull-tekton-pipeline-build-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Yongxuanzhang
Copy link
Member Author

This PR is divided into 3:

we could merge these 2 first
#6179
#6180

@XinruZhang
Copy link
Member

/assign

Hey Yongxuan, thank you for the PR and for dividing the PR into three, could you please fix the build failure for this PR :) ?

@Yongxuanzhang
Copy link
Member Author

Yongxuanzhang commented Feb 16, 2023

/assign

Hey Yongxuan, thank you for the PR and for dividing the PR into three, could you please fix the build failure for this PR :) ?

Yeah I will fix it when we merge the other 2 and rebase the code. This PR is currently held. Sorry I should mention it

@XinruZhang
Copy link
Member

Hi Yongxuan, wanna double check is this PR still active? I.e once the two PRs #6179 #6180 get merged, are you going to work on this PR to add the feature gate check or you are going to create a new PR for it?

@Yongxuanzhang
Copy link
Member Author

Hi Yongxuan, wanna double check is this PR still active? I.e once the two PRs #6179 #6180 get merged, are you going to work on this PR to add the feature gate check or you are going to create a new PR for it?

I plan to rebase PR after those 2 merged. But if it is of too much work I will probably open a new one.
Sometimes a new PR is easier. 😂

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2023
@tekton-robot
Copy link
Collaborator

@Yongxuanzhang: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Yongxuanzhang
Copy link
Member Author

Move to #6280 instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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
Development

Successfully merging this pull request may close these issues.

improve error message when indexing into an array is used with stable api
6 participants