Repo package cleanups/refactors #2387

justaugustus · 2021-01-30T22:29:52Z

I've reviewed a few PRs recently that I realized had issues that would've been caught by golangci-lint.

Done

Follow-ups

stop calling directories hack/ (use scripts/)
split tests into:
- go
- content verification
- build verification
migrate kepify to cobra
plumb logrus (replace fmt)
refactor out Person/Reviewer field
no references to API in commands (means we haven't encapsulated properly)
use k/release github token package, if exists
inconsistent flag choices

CI

use Makefile instead of hack/verify.sh in CI
need py3 for boilerplate script
need docker for shellcheck

k/release

update k/repo-infra version
go1.16 go.mod

Handoff (@jeremyrickard)

Not ready for review yet, but:
cc: @kubernetes/enhancements
/hold

justaugustus · 2021-01-30T22:32:18Z

(don't review yet and ignore the CI failures... this will get worse before it gets better 🙃)

n3wscott · 2021-01-31T05:02:37Z

cmd/kepctl/cmd/create.go

+	},
+}
+
+func init() {


The use of init means no one can cherry pick this command to remix their own cli. I prefer explicitly adding commands and in this way: https://github.com/kubernetes-sigs/zeitgeist/blob/master/buoy/commands/commands.go

n3wscott · 2021-01-31T05:05:10Z

cmd/kepctl/cmd/promote.go

+	"k8s.io/enhancements/pkg/kepctl"
+)
+
+// TODO: Struct literal instead?


I prefer no struct and have it be local to a function. https://github.com/kubernetes-sigs/zeitgeist/blob/master/buoy/commands/exists.go#L30 and this lets you reuse common flag defs like https://github.com/n3wscott/bujo/blob/master/pkg/commands/get.go#L89

justaugustus · 2021-02-01T01:46:31Z

(Rebased on top of #2388, which pulls out a lot of the non-go changes.)

justaugustus · 2021-02-01T04:58:55Z

I started rewriting the validation logic with github.com/go-playground/validator/v10.

82a4357 shows that off with a fake required field:

 --- FAIL: TestValidation (0.09s)
    --- FAIL: TestValidation/../keps/sig-api-machinery/1904-efficient-watch-resumption/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	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
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-api-machinery/1904-efficient-watch-resumption/kep.yaml
    --- FAIL: TestValidation/../keps/sig-apps/2214-indexed-job/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	Error:      	Expected nil, but got: validating PRR: running validation: Key: 'PRRApproval.Beta.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
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-apps/2214-indexed-job/kep.yaml
    --- FAIL: TestValidation/../keps/sig-apps/2255-pod-cost/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	Error:      	Expected nil, but got: validating PRR: running validation: Key: 'PRRApproval.Beta.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
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-apps/2255-pod-cost/kep.yaml
    --- FAIL: TestValidation/../keps/sig-apps/592-ttl-after-finish/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	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
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-apps/592-ttl-after-finish/kep.yaml
    --- FAIL: TestValidation/../keps/sig-apps/85-Graduate-PDB-to-Stable/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	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.Beta.Approver' Error:Field validation for 'Approver' failed on the 'required' tag
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-apps/85-Graduate-PDB-to-Stable/kep.yaml
    --- FAIL: TestValidation/../keps/sig-auth/1393-oidc-discovery/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	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.Beta.Approver' Error:Field validation for 'Approver' failed on the 'required' tag
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-auth/1393-oidc-discovery/kep.yaml
    --- FAIL: TestValidation/../keps/sig-network/2079-network-policy-port-range/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	Error:      	Expected nil, but got: validating PRR: running validation: Key: 'PRRApproval.Beta.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
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-network/2079-network-policy-port-range/kep.yaml
    --- FAIL: TestValidation/../keps/sig-network/2200-externalips-admission/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	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.Beta.Approver' Error:Field validation for 'Approver' failed on the 'required' tag
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-network/2200-externalips-admission/kep.yaml
    --- FAIL: TestValidation/../keps/sig-scheduling/1923-prefer-nominated-node/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	Error:      	Expected nil, but got: validating PRR: running validation: Key: 'PRRApproval.Beta.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
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-scheduling/1923-prefer-nominated-node/kep.yaml
    --- FAIL: TestValidation/../keps/sig-storage/1412-immutable-secrets-and-configmaps/kep.yaml (0.00s)
        metadata_test.go:126: 
            	Error Trace:	metadata_test.go:126
            	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.Beta.Approver' Error:Field validation for 'Approver' failed on the 'required' tag
            	            	Key: 'PRRApproval.FakeRequiredField' Error:Field validation for 'FakeRequiredField' failed on the 'required' tag
            	Test:       	TestValidation/../keps/sig-storage/1412-immutable-secrets-and-configmaps/kep.yaml
FAIL
FAIL	command-line-arguments	0.104s
FAIL
FAILED   verify-kep-metadata.sh	4s

(not quite there yet, but getting closer!)

Signed-off-by: Stephen Augustus <foo@auggie.dev>

justaugustus · 2021-02-02T15:25:03Z

/hold cancel
(rebased to pull in #2397)

jeremyrickard · 2021-02-02T15:28:20Z

/lgtm

I took a look at this, I think it sets us up for a good state for future work. Thanks @justaugustus

justaugustus · 2021-02-02T15:28:53Z

/pony party

k8s-ci-robot · 2021-02-02T15:28:59Z

@justaugustus:

In response to this:

/pony party

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.

palnabarun · 2021-02-02T15:29:25Z

Thank you @justaugustus 👍🏽

/lgtm
/honk

k8s-ci-robot · 2021-02-02T15:29:26Z

@palnabarun:

In response to this:

Thank you @justaugustus 👍🏽

/lgtm
/honk

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.

wojtek-t · 2021-02-02T17:43:12Z

pkg/legacy/keps/proposals.go

+			proposal.Error = errors.Wrap(err, "error validating KEP metadata")
+			return proposal
+		}
+	*/


@justaugustus - why this was commented out? We no longer validate e.g. if PRR approvers are from PRR approvers, etc....

@jeremyrickard

@johnbelamaric @deads2k - FYI

@wojtek-t -- Discussing on #enhancements and will send an updated PR later.

Thanks for flagging this!

This reverts commit 18903e5, reversing changes made to 5d6d935.

Revert "Merge pull request #2387 from justaugustus/cleanup"

k8s-ci-robot requested review from johnbelamaric and lasomethingsomething January 30, 2021 22:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2021

justaugustus mentioned this pull request Jan 30, 2021

query_test.go: Testcases are written for helper methods #2376

Closed

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2021

justaugustus added area/enhancements Issues or PRs related to the Enhancements subproject and removed sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. labels Jan 31, 2021

k8s-ci-robot added the sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. label Jan 31, 2021

n3wscott reviewed Jan 31, 2021

View reviewed changes

justaugustus mentioned this pull request Feb 1, 2021

Update code OWNERS and add more verify scripts #2388

Merged

justaugustus force-pushed the cleanup branch from 9db73ae to 7e62539 Compare February 1, 2021 01:44

justaugustus force-pushed the cleanup branch 2 times, most recently from b78806b to 82a4357 Compare February 1, 2021 04:56

justaugustus force-pushed the cleanup branch 2 times, most recently from 2d4148b to faa7fce Compare February 1, 2021 07:06

justaugustus added this to the keps-beta milestone Feb 1, 2021

justaugustus force-pushed the cleanup branch from faa7fce to 4fcab68 Compare February 1, 2021 07:29

justaugustus changed the title ~~[WIP] Repo package cleanups~~ Repo package cleanups/refactors Feb 1, 2021

k8s-ci-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 1, 2021

justaugustus added 10 commits February 2, 2021 10:03

go.mod: Add k/release/pkg and logrus

acc88a2

Signed-off-by: Stephen Augustus <foo@auggie.dev>

kepctl: Drop helper function and plumb common args

b3d3fdc

Signed-off-by: Stephen Augustus <foo@auggie.dev>

kepctl: Finish initial refactor

aed343d

Signed-off-by: Stephen Augustus <foo@auggie.dev>

test: Init with KEP metadata test from cmd/kepval

322997e

Signed-off-by: Stephen Augustus <foo@auggie.dev>

api: Add Document interface and PRR approval types

0e78730

Signed-off-by: Stephen Augustus <foo@auggie.dev>

pkg: Move kepval packages to legacy

eb2dba9

Signed-off-by: Stephen Augustus <foo@auggie.dev>

pkg: Replace kepval/** imports with legacy packages

ea111a0

Signed-off-by: Stephen Augustus <foo@auggie.dev>

test: Fixup PRR approval path

7fb91eb

Signed-off-by: Stephen Augustus <foo@auggie.dev>

api: Validate PRRs by field

8ffe0d2

Signed-off-by: Stephen Augustus <foo@auggie.dev>

api: Validate KEPs by field

685eecf

Signed-off-by: Stephen Augustus <foo@auggie.dev>

justaugustus force-pushed the cleanup branch from a0498ef to 685eecf Compare February 2, 2021 15:19

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2021

k8s-ci-robot merged commit 18903e5 into kubernetes:master Feb 2, 2021

k8s-ci-robot modified the milestones: keps-beta, v1.21 Feb 2, 2021

wojtek-t reviewed Feb 2, 2021

View reviewed changes

justaugustus mentioned this pull request Feb 2, 2021

api: Update field validations for Documents #2409

Merged

palnabarun mentioned this pull request Feb 4, 2021

WIP: Prototype for Receipts workflow #2424

Closed

11 tasks

justaugustus added a commit to justaugustus/enhancements that referenced this pull request Feb 4, 2021

Revert "Merge pull request kubernetes#2387 from justaugustus/cleanup"

0d6484e

This reverts commit 18903e5, reversing changes made to 5d6d935.

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #2425 from justaugustus/refactor-revert

3e2c87b

Revert "Merge pull request #2387 from justaugustus/cleanup"

justaugustus mentioned this pull request Feb 20, 2021

Repo package cleanups/refactors (part two) #2528

Merged

justaugustus mentioned this pull request Jan 22, 2022

Create an importable version cobra.Command kubernetes-sigs/release-utils#23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repo package cleanups/refactors #2387

Repo package cleanups/refactors #2387

justaugustus commented Jan 30, 2021 •

edited

Loading

justaugustus commented Jan 30, 2021

n3wscott Jan 31, 2021

n3wscott Jan 31, 2021

justaugustus commented Feb 1, 2021

justaugustus commented Feb 1, 2021 •

edited

Loading

justaugustus commented Feb 2, 2021

jeremyrickard commented Feb 2, 2021

justaugustus commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

palnabarun commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

wojtek-t Feb 2, 2021

justaugustus Feb 2, 2021

Repo package cleanups/refactors #2387

Repo package cleanups/refactors #2387

Conversation

justaugustus commented Jan 30, 2021 • edited Loading

Done

Follow-ups

CI

k/release

Handoff (@jeremyrickard)

justaugustus commented Jan 30, 2021

n3wscott Jan 31, 2021

Choose a reason for hiding this comment

n3wscott Jan 31, 2021

Choose a reason for hiding this comment

justaugustus commented Feb 1, 2021

justaugustus commented Feb 1, 2021 • edited Loading

justaugustus commented Feb 2, 2021

jeremyrickard commented Feb 2, 2021

justaugustus commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

palnabarun commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

wojtek-t Feb 2, 2021

Choose a reason for hiding this comment

justaugustus Feb 2, 2021

Choose a reason for hiding this comment

justaugustus commented Jan 30, 2021 •

edited

Loading

justaugustus commented Feb 1, 2021 •

edited

Loading