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

api: Update field validations for Documents #2409

Merged
merged 18 commits into from
Feb 10, 2021
Merged

api: Update field validations for Documents #2409

merged 18 commits into from
Feb 10, 2021

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Feb 2, 2021 •
edited
Loading

Addresses @wojtek-t's feedback in #2387 (comment).
(This is WIP, but I'm opening the PR and tagging people so you can track progress and see CI go green.)

Part of #2348.
Fixes #2470.

Signed-off-by: Stephen Augustus [email protected]

PRR approvers:
@wojtek-t @johnbelamaric @deads2k

cc: @kubernetes/enhancements
ref: https://kubernetes.slack.com/archives/C1L57L91V/p1612288049074600

@justaugustus justaugustus added this to the keps-beta milestone Feb 2, 2021
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 2, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/enhancements Issues or PRs related to the Enhancements subproject kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Feb 2, 2021
@justaugustus
Copy link
Member Author

This is helping to expose some problems with the current test as well...

From a snippet of the error message in the first presubmit run:

 --- FAIL: TestValidation (0.10s)
    --- FAIL: TestValidation/../keps/sig-api-machinery/1904-efficient-watch-resumption/kep.yaml (0.00s)
        metadata_test.go:104: 
            	Error Trace:	metadata_test.go:104
            	Error:      	Expected nil, but got: validating PRR: running validation: Key: 'PRRApproval.Alpha.Approver' Error:Field validation for 'Approver' failed on the 'required' tag
            	            	Key: 'PRRApproval.Stable.Approver' Error:Field validation for 'Approver' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-api-machinery/1904-efficient-watch-resumption/kep.yaml

you can see that it's attempting to validate a PRR field from a KEP metadata file, which is incorrect.

@wojtek-t
Copy link
Member

wojtek-t commented Feb 3, 2021

you can see that it's attempting to validate a PRR field from a KEP metadata file, which is incorrect.

It's not incorrect. I do NOT want the all fields to be required:

  • we don't want to backfill everything
  • it serves as a stamp of approval for a given stage - so we don't want this to be filled for future stages

The validation that I want is:
"If it is set, the value can only be a valid PRR approver login"

That's why I said that you won't easily be able to do that validation without custom one...

@justaugustus justaugustus mentioned this pull request Feb 3, 2021
Merged
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 3, 2021
This was referenced Feb 3, 2021
Merged
Merged
@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 3, 2021
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2021
@justaugustus justaugustus mentioned this pull request Feb 10, 2021
Merged
@justaugustus
Revert "Merge pull request kubernetes#2425 from justaugustus/refactor…
8a5c584 
…-revert"

This reverts commit 3e2c87b, reversing
changes made to 0a495c8.
@justaugustus
api: Make "title" and "owning-sig" fields required in KEPs
e5d51d1 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
api: Make "kep-number" and "approver" fields required for PRR approvals
4d00dec 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
pkg/kepval: Move directory validation functionality to package
19a694b 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
kepval: Begin coalescing non-blocking warnings in test
813a97f 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
api: Make "status" field required in KEPs
d7a4f72 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
api: Move group helpers closer to the validation logic
1f08547 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
Ensure PRR approver is actually a valid approver
96f579a 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
kepval: Retrieve absolute paths for KEP and PRR directories
0d99463 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
kepval: Use a map to coalesce errors based on filenames
97646d0 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
kepval: Fix PRR approver search
7e08693 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
kepval: Strip @ from PRR approver during validation
183d079 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
kepval: Temporarily skip PRR approval validation if "stage" is missing
6803980 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
pkg/legacy: Restore helpers.go
68d4128 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
hack/verify-kep-metadata: Make go test verbose
56ec6b9 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
api: Add interface directives for counterfeiter
6a8ccc9 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
generated: Run make update-mocks
ce64dc5 
Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
api: Add error handling and test for retrieving a stage's PRR approver
82b616d 
Signed-off-by: Stephen Augustus <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justaugustus

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2021
@jeremyrickard
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit f4d6220 into kubernetes:master Feb 10, 2021
@k8s-ci-robot k8s-ci-robot modified the milestones: keps-beta, v1.21 Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyrickard jeremyrickard Awaiting requested review from jeremyrickard

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

@palnabarun palnabarun Awaiting requested review from palnabarun

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@mrbobbytables mrbobbytables

@johnbelamaric johnbelamaric

@wojtek-t wojtek-t

@jeremyrickard jeremyrickard

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/enhancements Issues or PRs related to the Enhancements subproject area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

KEP validation seems to be brittle?
7 participants
@justaugustus @wojtek-t @jeremyrickard @fejta-bot @k8s-ci-robot @mrbobbytables @johnbelamaric