From b80efaf821bcf447f3c9d935af3f6e8a50e1f376 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 16:07:39 -0500 Subject: [PATCH 01/19] lint: Resolve simple style warnings Signed-off-by: Stephen Augustus --- cmd/kepctl/create.go | 2 +- cmd/kepctl/main.go | 4 +++- cmd/kepify/main.go | 6 ++++-- pkg/kepctl/create.go | 5 +---- pkg/kepctl/create_test.go | 3 +-- pkg/kepctl/kepctl.go | 20 +++++++++++--------- pkg/kepctl/kepctl_test.go | 1 - pkg/kepctl/query.go | 4 ++-- pkg/kepctl/query_test.go | 6 +++--- pkg/kepval/keps/validations/yaml.go | 2 +- pkg/kepval/keps/validations/yaml_test.go | 3 +-- pkg/kepval/prrs/approvals.go | 1 + pkg/kepval/prrs/validations/yaml.go | 4 +--- pkg/kepval/prrs/validations/yaml_test.go | 9 ++++----- pkg/kepval/util/helpers.go | 5 ++--- 15 files changed, 36 insertions(+), 39 deletions(-) diff --git a/cmd/kepctl/create.go b/cmd/kepctl/create.go index 8718b83941f..828151354f4 100644 --- a/cmd/kepctl/create.go +++ b/cmd/kepctl/create.go @@ -18,11 +18,11 @@ package main import ( "github.com/spf13/cobra" + "k8s.io/enhancements/pkg/kepctl" ) func buildCreateCommand(k *kepctl.Client) *cobra.Command { - opts := kepctl.CreateOpts{} cmd := &cobra.Command{ Use: "create [KEP]", diff --git a/cmd/kepctl/main.go b/cmd/kepctl/main.go index 7a37cb28748..894d457e33e 100644 --- a/cmd/kepctl/main.go +++ b/cmd/kepctl/main.go @@ -21,6 +21,7 @@ import ( "os" "github.com/spf13/cobra" + "k8s.io/enhancements/pkg/kepctl" ) @@ -42,7 +43,8 @@ func buildMainCommand() (*cobra.Command, error) { if err != nil { return nil, err } - var rootCmd = &cobra.Command{ + + rootCmd := &cobra.Command{ Use: "kepctl", Short: "kepctl helps you build keps", } diff --git a/cmd/kepify/main.go b/cmd/kepify/main.go index 133f982ae3b..4ee5a778203 100644 --- a/cmd/kepify/main.go +++ b/cmd/kepify/main.go @@ -111,14 +111,16 @@ func parseFiles(files []string) (api.Proposals, error) { parser := &keps.Parser{} file, err := os.Open(filename) if err != nil { - return nil, fmt.Errorf("could not open file: %v\n", err) + return nil, fmt.Errorf("could not open file: %v", err) } + defer file.Close() kep := parser.Parse(file) // if error is nil we can move on if kep.Error != nil { - return nil, fmt.Errorf("%v has an error: %q\n", filename, kep.Error.Error()) + return nil, fmt.Errorf("%v has an error: %q", filename, kep.Error.Error()) } + fmt.Printf(">>>> parsed file successfully: %s\n", filename) proposals.AddProposal(kep) } diff --git a/pkg/kepctl/create.go b/pkg/kepctl/create.go index 9942434a736..718d03c3ad9 100644 --- a/pkg/kepctl/create.go +++ b/pkg/kepctl/create.go @@ -48,7 +48,7 @@ func (c *CreateOpts) Validate(args []string) error { return err } if len(c.PRRApprovers) == 0 { - return errors.New("Must provide at least one PRR Approver") + return errors.New("must provide at least one PRR Approver") } return nil } @@ -56,7 +56,6 @@ func (c *CreateOpts) Validate(args []string) error { // Create builds a new KEP based on the README.md and kep.yaml templates in the // path specified by the command args. CreateOpts is used to populate the template func (c *Client) Create(opts CreateOpts) error { - fmt.Fprintf(c.Out, "Creating KEP %s %s %s\n", opts.SIG, opts.Number, opts.Name) repoPath, err := c.findEnhancementsRepo(opts.CommonArgs) fmt.Fprintf(c.Out, "Looking for enhancements repo in %s\n", repoPath) @@ -120,7 +119,6 @@ func updateTemplate(t *api.Proposal, opts CreateOpts) { if len(opts.PRRApprovers) > 0 { t.PRRApprovers = updatePersonReference(opts.PRRApprovers) } - } func updatePersonReference(names []string) []string { @@ -135,7 +133,6 @@ func updatePersonReference(names []string) []string { } func (c *Client) createKEP(kep *api.Proposal, opts CreateOpts) error { - fmt.Fprintf(c.Out, "Generating new KEP %s in %s ===>\n", opts.Name, opts.SIG) err := c.writeKEP(kep, opts.CommonArgs) diff --git a/pkg/kepctl/create_test.go b/pkg/kepctl/create_test.go index 9eadba9c064..2121de46a35 100644 --- a/pkg/kepctl/create_test.go +++ b/pkg/kepctl/create_test.go @@ -31,7 +31,6 @@ import ( ) func TestWriteKep(t *testing.T) { - testcases := []struct { name string kepFile string @@ -109,7 +108,6 @@ func TestWriteKep(t *testing.T) { } }) } - } type testClient struct { @@ -129,6 +127,7 @@ func newTestClient(t *testing.T, repoPath string) testClient { }, } + // TODO: Parameterize tc.addTemplate("kep.yaml") tc.addTemplate("README.md") return tc diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index d77fa3580a9..a9ef858d312 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -42,9 +42,9 @@ import ( ) type CommonArgs struct { - RepoPath string //override the default settings + RepoPath string // override the default settings TokenPath string - KEP string //KEP name sig-xxx/xxx-name + KEP string // KEP name sig-xxx/xxx-name Name string Number string SIG string @@ -56,11 +56,12 @@ func (c *CommonArgs) validateAndPopulateKEP(args []string) error { } if len(args) == 1 { kep := args[0] - re := regexp.MustCompile("([a-z\\-]+)/((\\d+)-.+)") + re := regexp.MustCompile(`([a-z\\-]+)/((\\d+)-.+)`) matches := re.FindStringSubmatch(kep) if matches == nil || len(matches) != 4 { return fmt.Errorf("invalid KEP name: %s", kep) } + c.KEP = kep c.SIG = matches[1] c.Number = matches[3] @@ -466,31 +467,32 @@ func (p *printConfig) Value(k *api.Proposal) string { return p.valueFunc(k) } +// TODO: Refactor out anonymous funcs var defaultConfig = map[string]printConfig{ "Authors": {"Authors", func(k *api.Proposal) string { return strings.Join(k.Authors, ", ") }}, "LastUpdated": {"Updated", func(k *api.Proposal) string { return k.LastUpdated }}, "SIG": {"SIG", func(k *api.Proposal) string { if strings.HasPrefix(k.OwningSIG, "sig-") { return k.OwningSIG[4:] - } else { - return k.OwningSIG } + + return k.OwningSIG }}, "Stage": {"Stage", func(k *api.Proposal) string { return k.Stage }}, "Status": {"Status", func(k *api.Proposal) string { return k.Status }}, "Title": {"Title", func(k *api.Proposal) string { if k.PRNumber == "" { return k.Title - } else { - return "PR#" + k.PRNumber + " - " + k.Title } + + return "PR#" + k.PRNumber + " - " + k.Title }}, "Link": {"Link", func(k *api.Proposal) string { if k.PRNumber == "" { return "https://git.k8s.io/enhancements/keps/" + k.OwningSIG + "/" + k.Name - } else { - return "https://github.com/kubernetes/enhancements/pull/" + k.PRNumber } + + return "https://github.com/kubernetes/enhancements/pull/" + k.PRNumber }}, } diff --git a/pkg/kepctl/kepctl_test.go b/pkg/kepctl/kepctl_test.go index 96e7c44e71a..4d8df0a7801 100644 --- a/pkg/kepctl/kepctl_test.go +++ b/pkg/kepctl/kepctl_test.go @@ -59,7 +59,6 @@ func TestValidate(t *testing.T) { } else { assert.EqualError(t, err, tc.err.Error()) } - }) } } diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index b0c209ce10e..fa5ada7214c 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -64,7 +64,7 @@ func (c *QueryOpts) Validate() error { return err } if len(sigs) == 0 { - return fmt.Errorf("No SIG matches any of the passed regular expressions") + return fmt.Errorf("no SIG matches any of the passed regular expressions") } c.SIG = sigs } else { @@ -77,7 +77,7 @@ func (c *QueryOpts) Validate() error { return fmt.Errorf("unsupported output format: %s. Valid values: %v", c.Output, SupportedOutputOpts) } - //TODO: check the valid values of stage, status, etc. + // TODO: check the valid values of stage, status, etc. return nil } diff --git a/pkg/kepctl/query_test.go b/pkg/kepctl/query_test.go index f3a8cf061e3..ff3bdaf4802 100644 --- a/pkg/kepctl/query_test.go +++ b/pkg/kepctl/query_test.go @@ -69,14 +69,14 @@ func TestValidateQueryOpt(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - var queryOpts = tc.queryOpts - var err = queryOpts.Validate() + queryOpts := tc.queryOpts + err := queryOpts.Validate() + if tc.err == nil { require.Nil(t, err) } else { require.NotNil(t, err, tc.err.Error()) } - }) } } diff --git a/pkg/kepval/keps/validations/yaml.go b/pkg/kepval/keps/validations/yaml.go index 7070a7eda34..f38efe425c8 100644 --- a/pkg/kepval/keps/validations/yaml.go +++ b/pkg/kepval/keps/validations/yaml.go @@ -30,7 +30,7 @@ var ( reStatus = regexp.MustCompile(strings.Join(statuses, "|")) stages = []string{"alpha", "beta", "stable"} reStages = regexp.MustCompile(strings.Join(stages, "|")) - reMilestone = regexp.MustCompile("v1\\.[1-9][0-9]*") + reMilestone = regexp.MustCompile(`v1\\.[1-9][0-9]*`) ) func ValidateStructure(parsed map[interface{}]interface{}) error { diff --git a/pkg/kepval/keps/validations/yaml_test.go b/pkg/kepval/keps/validations/yaml_test.go index 7f4d030cb51..dbb44102af5 100644 --- a/pkg/kepval/keps/validations/yaml_test.go +++ b/pkg/kepval/keps/validations/yaml_test.go @@ -162,8 +162,7 @@ func TestValidateStructureSuccess(t *testing.T) { } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - - // add required fields + // TODO: Add required fields tc.input["title"] = "this is a title" tc.input["owning-sig"] = "sig-architecture" diff --git a/pkg/kepval/prrs/approvals.go b/pkg/kepval/prrs/approvals.go index 6dde640e8c5..3993fe24b6e 100644 --- a/pkg/kepval/prrs/approvals.go +++ b/pkg/kepval/prrs/approvals.go @@ -23,6 +23,7 @@ import ( "github.com/pkg/errors" "gopkg.in/yaml.v2" + "k8s.io/enhancements/pkg/kepval/prrs/validations" ) diff --git a/pkg/kepval/prrs/validations/yaml.go b/pkg/kepval/prrs/validations/yaml.go index e2f7b81ee36..198e4dfca58 100644 --- a/pkg/kepval/prrs/validations/yaml.go +++ b/pkg/kepval/prrs/validations/yaml.go @@ -24,9 +24,7 @@ import ( "k8s.io/enhancements/pkg/kepval/util" ) -var ( - mandatoryKeys = []string{"kep-number"} -) +var mandatoryKeys = []string{"kep-number"} func ValidateStructure(parsed map[interface{}]interface{}) error { for _, key := range mandatoryKeys { diff --git a/pkg/kepval/prrs/validations/yaml_test.go b/pkg/kepval/prrs/validations/yaml_test.go index ef430d81269..a02ba46c366 100644 --- a/pkg/kepval/prrs/validations/yaml_test.go +++ b/pkg/kepval/prrs/validations/yaml_test.go @@ -20,14 +20,13 @@ import ( "testing" ) - func TestValidateStructureSuccess(t *testing.T) { testcases := []struct { name string input map[interface{}]interface{} }{ { - name: "all milestones optional", + name: "all milestones optional", input: map[interface{}]interface{}{}, }, { @@ -86,14 +85,14 @@ func TestValidateStructureFailures(t *testing.T) { }, }, { - name: "invalid milestone", + name: "invalid milestone", input: map[interface{}]interface{}{ "kep-number": "12345", - "alpha": "@wojtek-t", + "alpha": "@wojtek-t", }, }, { - name: "invalid approver", + name: "invalid approver", input: map[interface{}]interface{}{ "kep-number": "12345", "alpha": map[interface{}]interface{}{ diff --git a/pkg/kepval/util/helpers.go b/pkg/kepval/util/helpers.go index 8b5850cc90d..e790f50ffaf 100644 --- a/pkg/kepval/util/helpers.go +++ b/pkg/kepval/util/helpers.go @@ -105,9 +105,8 @@ func fetchPRRApprovers() ([]string, error) { return nil, fmt.Errorf("unable to read parse aliases content: %v", err) } var result []string - for _, approver := range config.Data["prod-readiness-approvers"] { - result = append(result, approver) - } + result = append(result, config.Data["prod-readiness-approvers"]...) + sort.Strings(result) return result, nil } From 6ab93179caf96592ce7d59a9f14b8782dad5f933 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 16:40:06 -0500 Subject: [PATCH 02/19] lint(gocritic): Fix trivial warnings Signed-off-by: Stephen Augustus --- cmd/kepify/main.go | 11 +++++++---- cmd/kepval/main_test.go | 4 +++- pkg/kepctl/create.go | 4 ++++ pkg/kepctl/kepctl.go | 2 +- pkg/kepctl/query.go | 2 +- pkg/kepval/keps/validations/yaml.go | 23 ++++++++++++++++++++++- 6 files changed, 38 insertions(+), 8 deletions(-) diff --git a/cmd/kepify/main.go b/cmd/kepify/main.go index 4ee5a778203..93887239a7a 100644 --- a/cmd/kepify/main.go +++ b/cmd/kepify/main.go @@ -44,7 +44,7 @@ func main() { flag.Usage = Usage flag.Parse() - if len(*dirPath) == 0 { + if *dirPath == "" { fmt.Fprintf(os.Stderr, "please specify the root directory for KEPs using '--dir'\n") os.Exit(1) } @@ -53,12 +53,12 @@ func main() { os.Exit(1) } - if len(*filePath) == 0 { + if *filePath == "" { fmt.Fprintf(os.Stderr, "please specify the file path for the output json using '--output'\n") os.Exit(1) } - // Find all the keps + // Find all of the KEPs files, err := findMarkdownFiles(dirPath) if err != nil { fmt.Fprintf(os.Stderr, "unable to find markdown files: %v\n", err) @@ -149,11 +149,13 @@ func printJSONOutput(filePath string, proposals api.Proposals) error { return nil } -/// ignore certain files in the keps/ subdirectory +// TODO: Consider replacing with a .kepignore file +// ignore certain files in the keps/ subdirectory func ignore(name string) bool { if !strings.HasSuffix(name, "md") { return true } + if name == "0023-documentation-for-images.md" || name == "0004-cloud-provider-template.md" || name == "0001a-meta-kep-implementation.md" || @@ -163,5 +165,6 @@ func ignore(name string) bool { name == "kep-faq.md" { return true } + return false } diff --git a/cmd/kepval/main_test.go b/cmd/kepval/main_test.go index 3f871be3448..a9ad5cc17dd 100644 --- a/cmd/kepval/main_test.go +++ b/cmd/kepval/main_test.go @@ -134,7 +134,7 @@ func TestValidation(t *testing.T) { } if len(stageMilestone) > 0 && stageMilestone >= "v1.21" { // PRR approval is needed. - if len(stagePRRApprover) == 0 { + if stagePRRApprover == "" { t.Errorf("PRR approval is required to target milestone %v (stage %v)", stageMilestone, kep.Stage) t.Errorf("For more details about PRR approval see: https://github.com/kubernetes/community/blob/master/sig-architecture/production-readiness.md") t.Errorf("To get PRR approval modify appropriately file %s and have this approved by PRR team", prrFilename) @@ -144,6 +144,8 @@ func TestValidation(t *testing.T) { } } +// TODO: Consider replacing with a .kepignore file +// TODO: Is this a duplicate of the package function? // ignore certain files in the keps/ subdirectory func ignore(dir, name string) bool { if dir == "../../keps/NNNN-kep-template" { diff --git a/pkg/kepctl/create.go b/pkg/kepctl/create.go index 718d03c3ad9..babcd5b44ad 100644 --- a/pkg/kepctl/create.go +++ b/pkg/kepctl/create.go @@ -99,8 +99,10 @@ func updateTemplate(t *api.Proposal, opts CreateOpts) { if !strings.HasPrefix(author, "@") { author = fmt.Sprintf("@%s", author) } + authors = append(authors, author) } + t.Authors = authors } @@ -139,10 +141,12 @@ func (c *Client) createKEP(kep *api.Proposal, opts CreateOpts) error { if err != nil { return fmt.Errorf("unable to create KEP: %s", err) } + path, err := c.findEnhancementsRepo(opts.CommonArgs) if err != nil { return err } + b, err := c.getReadmeTemplate(path) if err != nil { return fmt.Errorf("couldn't find README template: %s", err) diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index a9ef858d312..27c31e9c1d7 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -341,7 +341,7 @@ func (c *Client) loadKEPPullRequests(sig string) ([]*api.Proposal, error) { return allKEPs, nil } -func (c *Client) readKEP(repoPath string, sig, name string) (*api.Proposal, error) { +func (c *Client) readKEP(repoPath, sig, name string) (*api.Proposal, error) { kepPath := filepath.Join( repoPath, "keps", diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index fa5ada7214c..af6bf25df3b 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -185,7 +185,7 @@ func sliceContains(s []string, e string) bool { // returns all strings in vals that match at least one // regexp in regexps -func selectByRegexp(vals []string, regexps []string) ([]string, error) { +func selectByRegexp(vals, regexps []string) ([]string, error) { var matches []string for _, s := range vals { for _, r := range regexps { diff --git a/pkg/kepval/keps/validations/yaml.go b/pkg/kepval/keps/validations/yaml.go index f38efe425c8..041d363318e 100644 --- a/pkg/kepval/keps/validations/yaml.go +++ b/pkg/kepval/keps/validations/yaml.go @@ -58,40 +58,49 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { case []interface{}: return util.NewValueMustBeString(k, v) } + v, _ := value.(string) if !reStatus.Match([]byte(v)) { return util.NewValueMustBeOneOf(k, v, statuses) } + case "stage": switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) } + v, _ := value.(string) if !reStages.Match([]byte(v)) { return util.NewValueMustBeOneOf(k, v, stages) } + case "owning-sig": switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) } + v, _ := value.(string) index := sort.SearchStrings(listGroups, v) if index >= len(listGroups) || listGroups[index] != v { return util.NewValueMustBeOneOf(k, v, listGroups) } + // optional strings case "editor": if empty { continue } + fallthrough + case "title", "creation-date", "last-updated": switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) } + v, ok := value.(string) if ok && v == "" { return util.NewMustHaveOneValue(k) @@ -99,6 +108,7 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { if !ok { return util.NewValueMustBeString(k, v) } + // These are optional lists, so skip if there is no value case "participating-sigs", "replaces", "superseded-by", "see-also": if empty { @@ -109,18 +119,22 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { if len(v) == 0 { continue } + case interface{}: // This indicates an empty item is valid continue } + fallthrough + case "authors", "reviewers", "approvers": switch values := value.(type) { case []interface{}: if len(values) == 0 { return util.NewMustHaveAtLeastOneValue(k) } - if strings.ToLower(k) == "participating-sigs" { + + if strings.EqualFold(k, "participating-sigs") { for _, value := range values { v := value.(string) index := sort.SearchStrings(listGroups, v) @@ -129,9 +143,11 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { } } } + case interface{}: return util.NewValueMustBeListOfStrings(k, values) } + case "prr-approvers": switch values := value.(type) { case []interface{}: @@ -149,9 +165,11 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { return util.NewValueMustBeOneOf(k, v, prrApprovers) } } + case interface{}: return util.NewValueMustBeListOfStrings(k, values) } + case "latest-milestone": switch v := value.(type) { case []interface{}: @@ -161,12 +179,14 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { if !reMilestone.Match([]byte(v)) { return util.NewValueMustBeMilestone(k, v) } + case "milestone": switch v := value.(type) { case map[interface{}]interface{}: if err := validateMilestone(v); err != nil { return err } + case interface{}: return util.NewValueMustBeStruct(k, v) } @@ -189,6 +209,7 @@ func validateMilestone(parsed map[interface{}]interface{}) error { case []interface{}: return util.NewValueMustBeString(k, v) } + v, _ := value.(string) if !reMilestone.Match([]byte(v)) { return util.NewValueMustBeMilestone(k, v) From fb488182bffb562ac73e1e0cf2e0024330280294 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 17:03:56 -0500 Subject: [PATCH 03/19] lint(gocritic): Fix hugeParam warnings by passing by pointer Signed-off-by: Stephen Augustus --- cmd/kepctl/create.go | 2 +- cmd/kepctl/promote.go | 3 ++- cmd/kepctl/query.go | 2 +- pkg/kepctl/create.go | 16 ++++++++++------ pkg/kepctl/create_test.go | 8 +++++++- pkg/kepctl/kepctl.go | 9 ++++++--- pkg/kepctl/promote.go | 10 +++++++--- pkg/kepctl/query.go | 7 ++++--- pkg/kepval/keps/validations/yaml_test.go | 1 + 9 files changed, 39 insertions(+), 19 deletions(-) diff --git a/cmd/kepctl/create.go b/cmd/kepctl/create.go index 828151354f4..ff9ba872c5f 100644 --- a/cmd/kepctl/create.go +++ b/cmd/kepctl/create.go @@ -33,7 +33,7 @@ func buildCreateCommand(k *kepctl.Client) *cobra.Command { return opts.Validate(args) }, RunE: func(cmd *cobra.Command, args []string) error { - return k.Create(opts) + return k.Create(&opts) }, } diff --git a/cmd/kepctl/promote.go b/cmd/kepctl/promote.go index bd5bfdcf6b5..501eec4b96a 100644 --- a/cmd/kepctl/promote.go +++ b/cmd/kepctl/promote.go @@ -18,6 +18,7 @@ package main import ( "github.com/spf13/cobra" + "k8s.io/enhancements/pkg/kepctl" ) @@ -32,7 +33,7 @@ func buildPromoteCommand(k *kepctl.Client) *cobra.Command { return opts.Validate(args) }, RunE: func(cmd *cobra.Command, args []string) error { - return k.Promote(opts) + return k.Promote(&opts) }, } diff --git a/cmd/kepctl/query.go b/cmd/kepctl/query.go index f6efb0b29aa..e832393fd6d 100644 --- a/cmd/kepctl/query.go +++ b/cmd/kepctl/query.go @@ -35,7 +35,7 @@ func buildQueryCommand(k *kepctl.Client) *cobra.Command { return opts.Validate() }, RunE: func(cmd *cobra.Command, args []string) error { - return k.Query(opts) + return k.Query(&opts) }, } diff --git a/pkg/kepctl/create.go b/pkg/kepctl/create.go index babcd5b44ad..e5bbe4a03e8 100644 --- a/pkg/kepctl/create.go +++ b/pkg/kepctl/create.go @@ -55,13 +55,15 @@ func (c *CreateOpts) Validate(args []string) error { // Create builds a new KEP based on the README.md and kep.yaml templates in the // path specified by the command args. CreateOpts is used to populate the template -func (c *Client) Create(opts CreateOpts) error { +func (c *Client) Create(opts *CreateOpts) error { fmt.Fprintf(c.Out, "Creating KEP %s %s %s\n", opts.SIG, opts.Number, opts.Name) - repoPath, err := c.findEnhancementsRepo(opts.CommonArgs) + + repoPath, err := c.findEnhancementsRepo(&opts.CommonArgs) fmt.Fprintf(c.Out, "Looking for enhancements repo in %s\n", repoPath) if err != nil { return fmt.Errorf("unable to create KEP: %s", err) } + t, err := c.getKepTemplate(repoPath) if err != nil { return err @@ -73,6 +75,7 @@ func (c *Client) Create(opts CreateOpts) error { if err != nil { return err } + err = c.createKEP(t, opts) if err != nil { return err @@ -81,7 +84,7 @@ func (c *Client) Create(opts CreateOpts) error { return nil } -func updateTemplate(t *api.Proposal, opts CreateOpts) { +func updateTemplate(t *api.Proposal, opts *CreateOpts) { if opts.State != "" { t.Status = opts.State } @@ -134,15 +137,16 @@ func updatePersonReference(names []string) []string { return persons } -func (c *Client) createKEP(kep *api.Proposal, opts CreateOpts) error { +func (c *Client) createKEP(kep *api.Proposal, opts *CreateOpts) error { fmt.Fprintf(c.Out, "Generating new KEP %s in %s ===>\n", opts.Name, opts.SIG) - err := c.writeKEP(kep, opts.CommonArgs) + args := &opts.CommonArgs + err := c.writeKEP(kep, args) if err != nil { return fmt.Errorf("unable to create KEP: %s", err) } - path, err := c.findEnhancementsRepo(opts.CommonArgs) + path, err := c.findEnhancementsRepo(args) if err != nil { return err } diff --git a/pkg/kepctl/create_test.go b/pkg/kepctl/create_test.go index 2121de46a35..f57c088e1ff 100644 --- a/pkg/kepctl/create_test.go +++ b/pkg/kepctl/create_test.go @@ -83,19 +83,25 @@ func TestWriteKep(t *testing.T) { if repoPath == "" { repoPath = tc.opts.RepoPath } + repoPath = filepath.Join(tempDir, repoPath) c := newTestClient(t, repoPath) + b, err := ioutil.ReadFile(tc.kepFile) require.NoError(t, err) + var p api.Proposal err = yaml.Unmarshal(b, &p) require.NoError(t, err) + tc.opts.CommonArgs.RepoPath = repoPath - err = c.writeKEP(&p, tc.opts.CommonArgs) + err = c.writeKEP(&p, &tc.opts.CommonArgs) + if tc.expectError { assert.Error(t, err) } else { assert.NoError(t, err) + computedPath := filepath.Join(tempDir, tc.expectedPath) dirStat, err := os.Stat(computedPath) assert.NoError(t, err) diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index 27c31e9c1d7..0413c38ab9d 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -80,7 +80,7 @@ type Client struct { Err io.Writer } -func (c *Client) findEnhancementsRepo(opts CommonArgs) (string, error) { +func (c *Client) findEnhancementsRepo(opts *CommonArgs) (string, error) { dir := c.RepoPath if opts.RepoPath != "" { dir = opts.RepoPath @@ -116,7 +116,7 @@ func New(repo string) (*Client, error) { }, nil } -func (c *Client) SetGitHubToken(opts CommonArgs) error { +func (c *Client) SetGitHubToken(opts *CommonArgs) error { if opts.TokenPath != "" { token, err := ioutil.ReadFile(opts.TokenPath) if err != nil { @@ -428,11 +428,12 @@ func (c *Client) loadKEPFromOldStyle(kepPath string) (*api.Proposal, error) { return kep, nil } -func (c *Client) writeKEP(kep *api.Proposal, opts CommonArgs) error { +func (c *Client) writeKEP(kep *api.Proposal, opts *CommonArgs) error { path, err := c.findEnhancementsRepo(opts) if err != nil { return fmt.Errorf("unable to write KEP: %s", err) } + b, err := yaml.Marshal(kep) if err != nil { return fmt.Errorf("KEP is invalid: %s", err) @@ -447,8 +448,10 @@ func (c *Client) writeKEP(kep *api.Proposal, opts CommonArgs) error { ), os.ModePerm, ) + newPath := filepath.Join(path, "keps", opts.SIG, opts.Name, "kep.yaml") fmt.Fprintf(c.Out, "writing KEP to %s\n", newPath) + return ioutil.WriteFile(newPath, b, os.ModePerm) } diff --git a/pkg/kepctl/promote.go b/pkg/kepctl/promote.go index 17b420103e1..b91a763a764 100644 --- a/pkg/kepctl/promote.go +++ b/pkg/kepctl/promote.go @@ -33,26 +33,30 @@ func (c *PromoteOpts) Validate(args []string) error { // Promote changes the stage and target release for a specified KEP based on the // values specified in PromoteOpts is used to populate the template -func (c *Client) Promote(opts PromoteOpts) error { +func (c *Client) Promote(opts *PromoteOpts) error { fmt.Fprintf(c.Out, "Updating KEP %s/%s\n", opts.SIG, opts.Name) - repoPath, err := c.findEnhancementsRepo(opts.CommonArgs) + + repoPath, err := c.findEnhancementsRepo(&opts.CommonArgs) if err != nil { return fmt.Errorf("unable to promote KEP: %s", err) } + p, err := c.readKEP(repoPath, opts.SIG, opts.Name) if err != nil { return fmt.Errorf("unable to load KEP for promotion: %s", err) } + p.Stage = opts.Stage p.LatestMilestone = opts.Release p.LastUpdated = opts.Release - err = c.writeKEP(p, opts.CommonArgs) + err = c.writeKEP(p, &opts.CommonArgs) if err != nil { return fmt.Errorf("unable to write updated KEP: %s", err) } // TODO: Implement ticketing workflow artifact generation fmt.Fprintf(c.Out, "KEP %s/%s updated\n", opts.SIG, opts.Name) + return nil } diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index af6bf25df3b..929d7795c2f 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -83,7 +83,7 @@ func (c *QueryOpts) Validate() error { // Query searches the local repo and possibly GitHub for KEPs // that match the search criteria. -func (c *Client) Query(opts QueryOpts) error { +func (c *Client) Query(opts *QueryOpts) error { // if output format is json/yaml, suppress other outputs // json/yaml are structured formats, logging events which // do not conform to the spec will create formatting issues @@ -98,12 +98,12 @@ func (c *Client) Query(opts QueryOpts) error { fmt.Fprintf(c.Out, "Searching for KEPs...\n") } - repoPath, err := c.findEnhancementsRepo(opts.CommonArgs) + repoPath, err := c.findEnhancementsRepo(&opts.CommonArgs) if err != nil { return errors.Wrap(err, "unable to search KEPs") } - c.SetGitHubToken(opts.CommonArgs) + c.SetGitHubToken(&opts.CommonArgs) var allKEPs []*api.Proposal // load the KEPs for each listed SIG @@ -147,6 +147,7 @@ func (c *Client) Query(opts QueryOpts) error { if len(opts.Approver) > 0 && !atLeastOne(k.Approvers, allowedApprover) { continue } + keps = append(keps, k) } diff --git a/pkg/kepval/keps/validations/yaml_test.go b/pkg/kepval/keps/validations/yaml_test.go index dbb44102af5..b01368e899d 100644 --- a/pkg/kepval/keps/validations/yaml_test.go +++ b/pkg/kepval/keps/validations/yaml_test.go @@ -24,6 +24,7 @@ import ( "gopkg.in/yaml.v2" ) +// TODO: Fix field keys and flow flags type proposal struct { Title string `yaml:"title"` Authors []string `yaml:,flow` From 1cca8a7349762f1776729f2108979a2685bbd0bc Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 17:23:54 -0500 Subject: [PATCH 04/19] lint(prealloc): Preallocate slices Signed-off-by: Stephen Augustus --- pkg/kepctl/kepctl.go | 15 ++++++++++++--- pkg/kepctl/query.go | 4 ++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index 0413c38ab9d..1c1bdc64040 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -151,11 +151,14 @@ func (c *Client) getReadmeTemplate(repoPath string) ([]byte, error) { return ioutil.ReadFile(path) } +// TODO: Unused? +//golint:deadcode,unused func validateKEP(p *api.Proposal) error { b, err := yaml.Marshal(p) if err != nil { return err } + r := bytes.NewReader(b) parser := &keps.Parser{} @@ -163,6 +166,7 @@ func validateKEP(p *api.Proposal) error { if kep.Error != nil { return fmt.Errorf("kep is invalid: %s", kep.Error) } + return nil } @@ -254,18 +258,21 @@ func (c *Client) loadKEPPullRequests(sig string) ([]*api.Proposal, error) { opt.Page = resp.NextPage } - var kepPRs []*github.PullRequest + kepPRs := make([]*github.PullRequest, 10) for _, pr := range allPulls { foundKind, foundSIG := false, false sigLabel := strings.Replace(sig, "-", "/", 1) + for _, l := range pr.Labels { if *l.Name == "kind/kep" { foundKind = true } + if *l.Name == sigLabel { foundSIG = true } } + if !foundKind || !foundSIG { continue } @@ -500,12 +507,13 @@ var defaultConfig = map[string]printConfig{ } func DefaultPrintConfigs(names ...string) []PrintConfig { - var configs []PrintConfig + configs := make([]PrintConfig, 10) for _, n := range names { // copy to allow it to be tweaked by the caller c := defaultConfig[n] configs = append(configs, &c) } + return configs } @@ -516,10 +524,11 @@ func (c *Client) PrintTable(configs []PrintConfig, proposals []*api.Proposal) { table := tablewriter.NewWriter(c.Out) - var headers []string + headers := make([]string, 10) for _, c := range configs { headers = append(headers, c.Title()) } + table.SetHeader(headers) table.SetAlignment(tablewriter.ALIGN_LEFT) diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index 929d7795c2f..8b81f5b06df 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -105,7 +105,7 @@ func (c *Client) Query(opts *QueryOpts) error { c.SetGitHubToken(&opts.CommonArgs) - var allKEPs []*api.Proposal + allKEPs := make([]*api.Proposal, 10) // load the KEPs for each listed SIG for _, sig := range opts.SIG { // KEPs in the local filesystem @@ -130,7 +130,7 @@ func (c *Client) Query(opts *QueryOpts) error { allowedAuthor := sliceToMap(opts.Author) allowedApprover := sliceToMap(opts.Approver) - var keps []*api.Proposal + keps := make([]*api.Proposal, 10) for _, k := range allKEPs { if len(opts.Status) > 0 && !allowedStatus[k.Status] { continue From 6f691ebaf3cc5ed2894502861bbd7025dd03fbc4 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 18:13:00 -0500 Subject: [PATCH 05/19] lint(errcheck): Fix trivial warnings and nolint refactor targets Signed-off-by: Stephen Augustus --- pkg/kepctl/create.go | 9 +++++++-- pkg/kepctl/kepctl.go | 10 +++++++--- pkg/kepctl/kepctl_test.go | 4 +++- pkg/kepctl/query.go | 4 +++- pkg/kepval/keps/validations/yaml.go | 16 ++++++++++++++++ pkg/kepval/prrs/validations/yaml.go | 6 +++++- 6 files changed, 41 insertions(+), 8 deletions(-) diff --git a/pkg/kepctl/create.go b/pkg/kepctl/create.go index e5bbe4a03e8..64c8d9c5874 100644 --- a/pkg/kepctl/create.go +++ b/pkg/kepctl/create.go @@ -17,7 +17,6 @@ limitations under the License. package kepctl import ( - "errors" "fmt" "io/ioutil" "os" @@ -25,6 +24,8 @@ import ( "strings" "time" + "github.com/pkg/errors" + "k8s.io/enhancements/api" ) @@ -47,9 +48,11 @@ func (c *CreateOpts) Validate(args []string) error { if err != nil { return err } + if len(c.PRRApprovers) == 0 { return errors.New("must provide at least one PRR Approver") } + return nil } @@ -157,7 +160,9 @@ func (c *Client) createKEP(kep *api.Proposal, opts *CreateOpts) error { } newPath := filepath.Join(path, "keps", opts.SIG, opts.Name, "README.md") - ioutil.WriteFile(newPath, b, os.ModePerm) + if writeErr := ioutil.WriteFile(newPath, b, os.ModePerm); writeErr != nil { + return errors.Wrapf(writeErr, "writing KEP data to file") + } return nil } diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index 1c1bdc64040..1996e077db4 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -32,6 +32,7 @@ import ( "github.com/google/go-github/v32/github" "github.com/olekukonko/tablewriter" + "github.com/pkg/errors" "golang.org/x/oauth2" "gopkg.in/yaml.v2" @@ -122,8 +123,10 @@ func (c *Client) SetGitHubToken(opts *CommonArgs) error { if err != nil { return err } + c.Token = strings.Trim(string(token), "\n\r") } + return nil } @@ -152,7 +155,6 @@ func (c *Client) getReadmeTemplate(repoPath string) ([]byte, error) { } // TODO: Unused? -//golint:deadcode,unused func validateKEP(p *api.Proposal) error { b, err := yaml.Marshal(p) if err != nil { @@ -446,7 +448,7 @@ func (c *Client) writeKEP(kep *api.Proposal, opts *CommonArgs) error { return fmt.Errorf("KEP is invalid: %s", err) } - os.MkdirAll( + if mkErr := os.MkdirAll( filepath.Join( path, "keps", @@ -454,7 +456,9 @@ func (c *Client) writeKEP(kep *api.Proposal, opts *CommonArgs) error { opts.Name, ), os.ModePerm, - ) + ); mkErr != nil { + return errors.Wrapf(mkErr, "creating KEP directory") + } newPath := filepath.Join(path, "keps", opts.SIG, opts.Name, "kep.yaml") fmt.Fprintf(c.Out, "writing KEP to %s\n", newPath) diff --git a/pkg/kepctl/kepctl_test.go b/pkg/kepctl/kepctl_test.go index 4d8df0a7801..d2a72d65240 100644 --- a/pkg/kepctl/kepctl_test.go +++ b/pkg/kepctl/kepctl_test.go @@ -78,7 +78,9 @@ func TestFindLocalKEPs(t *testing.T) { }, } - c, _ := New("testdata") + c, clientErr := New("testdata") + require.Nil(t, clientErr) + for i, tc := range testcases { k := c.loadLocalKEPs("testdata", tc.sig) if len(k) != len(tc.keps) { diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index 8b81f5b06df..dbbad500738 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -103,7 +103,9 @@ func (c *Client) Query(opts *QueryOpts) error { return errors.Wrap(err, "unable to search KEPs") } - c.SetGitHubToken(&opts.CommonArgs) + if tokenErr := c.SetGitHubToken(&opts.CommonArgs); tokenErr != nil { + return errors.Wrapf(tokenErr, "setting GitHub token") + } allKEPs := make([]*api.Proposal, 10) // load the KEPs for each listed SIG diff --git a/pkg/kepval/keps/validations/yaml.go b/pkg/kepval/keps/validations/yaml.go index 041d363318e..16859609d17 100644 --- a/pkg/kepval/keps/validations/yaml.go +++ b/pkg/kepval/keps/validations/yaml.go @@ -59,6 +59,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { return util.NewValueMustBeString(k, v) } + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v, _ := value.(string) if !reStatus.Match([]byte(v)) { return util.NewValueMustBeOneOf(k, v, statuses) @@ -70,6 +72,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { return util.NewValueMustBeString(k, v) } + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v, _ := value.(string) if !reStages.Match([]byte(v)) { return util.NewValueMustBeOneOf(k, v, stages) @@ -81,6 +85,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { return util.NewValueMustBeString(k, v) } + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v, _ := value.(string) index := sort.SearchStrings(listGroups, v) if index >= len(listGroups) || listGroups[index] != v { @@ -136,6 +142,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { if strings.EqualFold(k, "participating-sigs") { for _, value := range values { + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v := value.(string) index := sort.SearchStrings(listGroups, v) if index >= len(listGroups) || listGroups[index] != v { @@ -154,6 +162,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { // prrApprovers must be sorted to use SearchStrings down below... sort.Strings(prrApprovers) for _, value := range values { + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v := value.(string) if len(v) > 0 && v[0] == '@' { // If "@" is appeneded at the beginning, remove it. @@ -175,6 +185,9 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { case []interface{}: return util.NewValueMustBeString(k, v) } + + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v, _ := value.(string) if !reMilestone.Match([]byte(v)) { return util.NewValueMustBeMilestone(k, v) @@ -192,6 +205,7 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { } } } + return nil } @@ -210,6 +224,8 @@ func validateMilestone(parsed map[interface{}]interface{}) error { return util.NewValueMustBeString(k, v) } + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v, _ := value.(string) if !reMilestone.Match([]byte(v)) { return util.NewValueMustBeMilestone(k, v) diff --git a/pkg/kepval/prrs/validations/yaml.go b/pkg/kepval/prrs/validations/yaml.go index 198e4dfca58..3fbdfb4aa85 100644 --- a/pkg/kepval/prrs/validations/yaml.go +++ b/pkg/kepval/prrs/validations/yaml.go @@ -75,11 +75,15 @@ func validateMilestone(parsed map[interface{}]interface{}) error { case []interface{}: return util.NewValueMustBeString(k, v) } + + // TODO(lint): Error return value is not checked (errcheck) + //nolint:errcheck v, _ := value.(string) if len(v) > 0 && v[0] == '@' { - // If "@" is appeneded at the beginning, remove it. + // If "@" is appended at the beginning, remove it. v = v[1:] } + index := sort.SearchStrings(prrApprovers, v) if index >= len(prrApprovers) || prrApprovers[index] != v { return util.NewValueMustBeOneOf(k, v, prrApprovers) From 83f6990d9f6aa91ae7fd32a3aa2e923e0c470658 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 18:39:33 -0500 Subject: [PATCH 06/19] lint: nolint refactor targets Signed-off-by: Stephen Augustus --- pkg/kepctl/create.go | 3 +++ pkg/kepctl/kepctl.go | 2 ++ pkg/kepval/keps/validations/yaml.go | 14 ++++++++++++++ pkg/kepval/keps/validations/yaml_test.go | 3 ++- pkg/kepval/prrs/validations/yaml.go | 4 ++++ 5 files changed, 25 insertions(+), 1 deletion(-) diff --git a/pkg/kepctl/create.go b/pkg/kepctl/create.go index 64c8d9c5874..7c3eeec1156 100644 --- a/pkg/kepctl/create.go +++ b/pkg/kepctl/create.go @@ -121,6 +121,9 @@ func updateTemplate(t *api.Proposal, opts *CreateOpts) { } t.OwningSIG = opts.SIG + + // TODO(lint): appendAssign: append result not assigned to the same slice (gocritic) + //nolint:gocritic t.ParticipatingSIGs = append(opts.SIGS, opts.SIG) t.Filename = opts.Name t.LastUpdated = "v1.19" diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index 1996e077db4..0fa57f0d7ba 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -178,6 +178,8 @@ func findLocalKEPMeta(repoPath, sig string) ([]string, error) { "keps", sig) + // TODO(lint): importShadow: shadow of imported from 'k8s.io/enhancements/pkg/kepval/keps' package 'keps' (gocritic) + //nolint:gocritic keps := []string{} // if the sig doesn't have a dir, it has no KEPs diff --git a/pkg/kepval/keps/validations/yaml.go b/pkg/kepval/keps/validations/yaml.go index 16859609d17..ae63f74e326 100644 --- a/pkg/kepval/keps/validations/yaml.go +++ b/pkg/kepval/keps/validations/yaml.go @@ -33,6 +33,8 @@ var ( reMilestone = regexp.MustCompile(`v1\\.[1-9][0-9]*`) ) +// TODO(lint): cyclomatic complexity 50 of func `ValidateStructure` is high (> 30) (gocyclo) +//nolint:gocyclo func ValidateStructure(parsed map[interface{}]interface{}) error { for _, key := range mandatoryKeys { if _, found := parsed[key]; !found { @@ -54,6 +56,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { // figure out the types switch strings.ToLower(k) { case "status": + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) @@ -67,6 +71,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { } case "stage": + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) @@ -80,6 +86,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { } case "owning-sig": + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) @@ -102,6 +110,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { fallthrough case "title", "creation-date", "last-updated": + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) @@ -181,6 +191,8 @@ func ValidateStructure(parsed map[interface{}]interface{}) error { } case "latest-milestone": + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) @@ -217,6 +229,8 @@ func validateMilestone(parsed map[interface{}]interface{}) error { return util.NewKeyMustBeString(k) } + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch strings.ToLower(k) { case "alpha", "beta", "stable": switch v := value.(type) { diff --git a/pkg/kepval/keps/validations/yaml_test.go b/pkg/kepval/keps/validations/yaml_test.go index b01368e899d..fec00298f75 100644 --- a/pkg/kepval/keps/validations/yaml_test.go +++ b/pkg/kepval/keps/validations/yaml_test.go @@ -24,7 +24,8 @@ import ( "gopkg.in/yaml.v2" ) -// TODO: Fix field keys and flow flags +// TODO(lint): Fix field keys and flow flags +//nolint:govet type proposal struct { Title string `yaml:"title"` Authors []string `yaml:,flow` diff --git a/pkg/kepval/prrs/validations/yaml.go b/pkg/kepval/prrs/validations/yaml.go index 3fbdfb4aa85..1e85912e5b2 100644 --- a/pkg/kepval/prrs/validations/yaml.go +++ b/pkg/kepval/prrs/validations/yaml.go @@ -69,8 +69,12 @@ func validateMilestone(parsed map[interface{}]interface{}) error { } // figure out the types + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch strings.ToLower(k) { case "approver": + // TODO(lint): singleCaseSwitch: should rewrite switch statement to if statement (gocritic) + //nolint:gocritic switch v := value.(type) { case []interface{}: return util.NewValueMustBeString(k, v) From da87f820b84913ddf5e6b12fb9deea2ae18b9fb6 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 18:46:39 -0500 Subject: [PATCH 07/19] pkg: Use testify/require instead of testify/assert Signed-off-by: Stephen Augustus --- pkg/kepctl/create_test.go | 13 ++++++------- pkg/kepctl/kepctl_test.go | 5 ++--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/kepctl/create_test.go b/pkg/kepctl/create_test.go index f57c088e1ff..58773bcbb8b 100644 --- a/pkg/kepctl/create_test.go +++ b/pkg/kepctl/create_test.go @@ -23,7 +23,6 @@ import ( "path/filepath" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/enhancements/api" @@ -98,19 +97,19 @@ func TestWriteKep(t *testing.T) { err = c.writeKEP(&p, &tc.opts.CommonArgs) if tc.expectError { - assert.Error(t, err) + require.Error(t, err) } else { - assert.NoError(t, err) + require.NoError(t, err) computedPath := filepath.Join(tempDir, tc.expectedPath) dirStat, err := os.Stat(computedPath) - assert.NoError(t, err) + require.NoError(t, err) require.NotNil(t, dirStat) - assert.True(t, dirStat.IsDir()) + require.True(t, dirStat.IsDir()) p := filepath.Join(computedPath, "kep.yaml") fileStat, err := os.Stat(p) - assert.NoError(t, err) - assert.NotNil(t, fileStat) + require.NoError(t, err) + require.NotNil(t, fileStat) } }) } diff --git a/pkg/kepctl/kepctl_test.go b/pkg/kepctl/kepctl_test.go index d2a72d65240..081b6abadc5 100644 --- a/pkg/kepctl/kepctl_test.go +++ b/pkg/kepctl/kepctl_test.go @@ -21,7 +21,6 @@ import ( "io/ioutil" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" @@ -55,9 +54,9 @@ func TestValidate(t *testing.T) { require.NoError(t, err) err = validateKEP(&p) if tc.err == nil { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tc.err.Error()) + require.EqualError(t, err, tc.err.Error()) } }) } From e8122df83c9b900697cd8b24dcf010bf4d1ec72f Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 19:03:51 -0500 Subject: [PATCH 08/19] go.mod: Upgrade to gopkg.in/yaml.v3 Signed-off-by: Stephen Augustus --- go.mod | 2 +- pkg/kepctl/kepctl.go | 2 +- pkg/kepctl/kepctl_test.go | 2 +- pkg/kepval/keps/proposals.go | 4 ++-- pkg/kepval/keps/validations/yaml_test.go | 2 +- pkg/kepval/prrs/approvals.go | 7 ++++--- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index cc102786cf5..dd4ae182eb1 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 golang.org/x/oauth2 v0.0.0-20210112200429-01de73cf58bd - gopkg.in/yaml.v2 v2.4.0 + gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c k8s.io/release v0.7.0 k8s.io/test-infra v0.0.0-20200813194141-e9678d500461 sigs.k8s.io/yaml v1.2.0 diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index 0fa57f0d7ba..e15ce2fe8fd 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -34,7 +34,7 @@ import ( "github.com/olekukonko/tablewriter" "github.com/pkg/errors" "golang.org/x/oauth2" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "k8s.io/enhancements/api" "k8s.io/enhancements/pkg/kepval/keps" diff --git a/pkg/kepctl/kepctl_test.go b/pkg/kepctl/kepctl_test.go index 081b6abadc5..6586ca302c7 100644 --- a/pkg/kepctl/kepctl_test.go +++ b/pkg/kepctl/kepctl_test.go @@ -22,7 +22,7 @@ import ( "testing" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "k8s.io/enhancements/api" ) diff --git a/pkg/kepval/keps/proposals.go b/pkg/kepval/keps/proposals.go index 79bb2d1bd72..e898263bd2c 100644 --- a/pkg/kepval/keps/proposals.go +++ b/pkg/kepval/keps/proposals.go @@ -25,7 +25,7 @@ import ( "strings" "github.com/pkg/errors" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "k8s.io/enhancements/api" "k8s.io/enhancements/pkg/kepval/keps/validations" @@ -75,7 +75,7 @@ func (p *Parser) Parse(in io.Reader) *api.Proposal { return proposal } - proposal.Error = yaml.UnmarshalStrict(metadata, proposal) + proposal.Error = yaml.Unmarshal(metadata, proposal) proposal.ID = hash(proposal.OwningSIG + ":" + proposal.Title) return proposal } diff --git a/pkg/kepval/keps/validations/yaml_test.go b/pkg/kepval/keps/validations/yaml_test.go index fec00298f75..46bc6e08dbd 100644 --- a/pkg/kepval/keps/validations/yaml_test.go +++ b/pkg/kepval/keps/validations/yaml_test.go @@ -21,7 +21,7 @@ import ( "html/template" "testing" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" ) // TODO(lint): Fix field keys and flow flags diff --git a/pkg/kepval/prrs/approvals.go b/pkg/kepval/prrs/approvals.go index 3993fe24b6e..61348515636 100644 --- a/pkg/kepval/prrs/approvals.go +++ b/pkg/kepval/prrs/approvals.go @@ -22,7 +22,7 @@ import ( "io" "github.com/pkg/errors" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" "k8s.io/enhancements/pkg/kepval/prrs/validations" ) @@ -65,7 +65,7 @@ func (p *Parser) Parse(in io.Reader) *Approval { // First do structural checks test := map[interface{}]interface{}{} if err := yaml.Unmarshal(body.Bytes(), test); err != nil { - approval.Error = errors.Wrap(err, "error unmarshaling YAML") + approval.Error = errors.Wrap(err, "error unmarshalling YAML") return approval } if err := validations.ValidateStructure(test); err != nil { @@ -73,7 +73,8 @@ func (p *Parser) Parse(in io.Reader) *Approval { return approval } - approval.Error = yaml.UnmarshalStrict(body.Bytes(), approval) + approval.Error = yaml.Unmarshal(body.Bytes(), approval) + return approval } From b64e5bfdcb35cb48e7980609cd71d7d8029a43ec Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 19:45:49 -0500 Subject: [PATCH 09/19] kepctl: Respect cobra directory layout Signed-off-by: Stephen Augustus --- cmd/kepctl/{ => cmd}/create.go | 2 +- cmd/kepctl/{ => cmd}/promote.go | 2 +- cmd/kepctl/{ => cmd}/query.go | 2 +- cmd/kepctl/cmd/root.go | 94 +++++++++++++++++++++++++++++++++ cmd/kepctl/main.go | 37 +------------ 5 files changed, 99 insertions(+), 38 deletions(-) rename cmd/kepctl/{ => cmd}/create.go (99%) rename cmd/kepctl/{ => cmd}/promote.go (99%) rename cmd/kepctl/{ => cmd}/query.go (99%) create mode 100644 cmd/kepctl/cmd/root.go diff --git a/cmd/kepctl/create.go b/cmd/kepctl/cmd/create.go similarity index 99% rename from cmd/kepctl/create.go rename to cmd/kepctl/cmd/create.go index ff9ba872c5f..9643e288621 100644 --- a/cmd/kepctl/create.go +++ b/cmd/kepctl/cmd/create.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package main +package cmd import ( "github.com/spf13/cobra" diff --git a/cmd/kepctl/promote.go b/cmd/kepctl/cmd/promote.go similarity index 99% rename from cmd/kepctl/promote.go rename to cmd/kepctl/cmd/promote.go index 501eec4b96a..99882c91df1 100644 --- a/cmd/kepctl/promote.go +++ b/cmd/kepctl/cmd/promote.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package main +package cmd import ( "github.com/spf13/cobra" diff --git a/cmd/kepctl/query.go b/cmd/kepctl/cmd/query.go similarity index 99% rename from cmd/kepctl/query.go rename to cmd/kepctl/cmd/query.go index e832393fd6d..e651153607d 100644 --- a/cmd/kepctl/query.go +++ b/cmd/kepctl/cmd/query.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package main +package cmd import ( "fmt" diff --git a/cmd/kepctl/cmd/root.go b/cmd/kepctl/cmd/root.go new file mode 100644 index 00000000000..25c7823bddb --- /dev/null +++ b/cmd/kepctl/cmd/root.go @@ -0,0 +1,94 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cmd + +import ( + "fmt" + "os" + + "github.com/sirupsen/logrus" + "github.com/spf13/cobra" + + "k8s.io/enhancements/pkg/kepctl" + "k8s.io/release/pkg/log" +) + +// rootCmd represents the base command when called without any subcommands +var rootCmd = &cobra.Command{ + Use: "kepctl", + Short: "kepctl helps you build keps", + PersistentPreRunE: initLogging, +} + +type rootOptions struct { + logLevel string +} + +var rootOpts = &rootOptions{} + +// Execute adds all child commands to the root command and sets flags appropriately. +// This is called by main.main(). It only needs to happen once to the rootCmd. +func Execute() { + if err := rootCmd.Execute(); err != nil { + logrus.Fatal(err) + } +} + +func init() { + rootCmd.PersistentFlags().StringVar( + &rootOpts.logLevel, + "log-level", + "info", + fmt.Sprintf("the logging verbosity, either %s", log.LevelNames()), + ) +} + +func initLogging(*cobra.Command, []string) error { + return log.SetupGlobalLogger(rootOpts.logLevel) +} + +// TODO: Refactor/remove below + +func main() { + cmd, err := buildMainCommand() + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + if err := cmd.Execute(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} + +func buildMainCommand() (*cobra.Command, error) { + repoPath := os.Getenv("ENHANCEMENTS_PATH") + k, err := kepctl.New(repoPath) + if err != nil { + return nil, err + } + + rootCmd := &cobra.Command{ + Use: "kepctl", + Short: "kepctl helps you build keps", + } + + rootCmd.AddCommand(buildCreateCommand(k)) + rootCmd.AddCommand(buildPromoteCommand(k)) + rootCmd.AddCommand(buildQueryCommand(k)) + return rootCmd, nil +} diff --git a/cmd/kepctl/main.go b/cmd/kepctl/main.go index 894d457e33e..568af49a197 100644 --- a/cmd/kepctl/main.go +++ b/cmd/kepctl/main.go @@ -16,41 +16,8 @@ limitations under the License. package main -import ( - "fmt" - "os" - - "github.com/spf13/cobra" - - "k8s.io/enhancements/pkg/kepctl" -) +import "k8s.io/enhancements/cmd/kepctl/cmd" func main() { - cmd, err := buildMainCommand() - if err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) - } - if err := cmd.Execute(); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) - } -} - -func buildMainCommand() (*cobra.Command, error) { - repoPath := os.Getenv("ENHANCEMENTS_PATH") - k, err := kepctl.New(repoPath) - if err != nil { - return nil, err - } - - rootCmd := &cobra.Command{ - Use: "kepctl", - Short: "kepctl helps you build keps", - } - - rootCmd.AddCommand(buildCreateCommand(k)) - rootCmd.AddCommand(buildPromoteCommand(k)) - rootCmd.AddCommand(buildQueryCommand(k)) - return rootCmd, nil + cmd.Execute() } From acc88a2c285659ae36f097b7b8820866328f9036 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 19:47:52 -0500 Subject: [PATCH 10/19] go.mod: Add k/release/pkg and logrus Signed-off-by: Stephen Augustus --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index dd4ae182eb1..69cf55a0080 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/olekukonko/tablewriter v0.0.4 github.com/pkg/errors v0.9.1 github.com/psampaz/go-mod-outdated v0.7.0 + github.com/sirupsen/logrus v1.7.0 github.com/spf13/cobra v1.1.1 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 From b3d3fdcdd2e4ab77dcc846902f6bf9a37723d314 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 20:51:18 -0500 Subject: [PATCH 11/19] kepctl: Drop helper function and plumb common args Signed-off-by: Stephen Augustus --- cmd/kepctl/cmd/root.go | 57 +++++++++++++----------------------------- cmd/kepctl/helper.go | 31 ----------------------- pkg/kepctl/kepctl.go | 12 ++++++--- 3 files changed, 26 insertions(+), 74 deletions(-) delete mode 100644 cmd/kepctl/helper.go diff --git a/cmd/kepctl/cmd/root.go b/cmd/kepctl/cmd/root.go index 25c7823bddb..e9b7a378798 100644 --- a/cmd/kepctl/cmd/root.go +++ b/cmd/kepctl/cmd/root.go @@ -30,15 +30,11 @@ import ( // rootCmd represents the base command when called without any subcommands var rootCmd = &cobra.Command{ Use: "kepctl", - Short: "kepctl helps you build keps", + Short: "kepctl helps you build KEPs", PersistentPreRunE: initLogging, } -type rootOptions struct { - logLevel string -} - -var rootOpts = &rootOptions{} +var rootOpts = &kepctl.CommonArgs{} // Execute adds all child commands to the root command and sets flags appropriately. // This is called by main.main(). It only needs to happen once to the rootCmd. @@ -50,45 +46,28 @@ func Execute() { func init() { rootCmd.PersistentFlags().StringVar( - &rootOpts.logLevel, + &rootOpts.LogLevel, "log-level", "info", fmt.Sprintf("the logging verbosity, either %s", log.LevelNames()), ) -} -func initLogging(*cobra.Command, []string) error { - return log.SetupGlobalLogger(rootOpts.logLevel) -} - -// TODO: Refactor/remove below + // TODO: This should be defaulted in the package instead + rootCmd.PersistentFlags().StringVar( + &rootOpts.RepoPath, + "repo-path", + os.Getenv("ENHANCEMENTS_PATH"), + "path to kubernetes/enhancements", + ) -func main() { - cmd, err := buildMainCommand() - if err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) - } - if err := cmd.Execute(); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) - } + rootCmd.PersistentFlags().StringVar( + &rootOpts.TokenPath, + "gh-token-path", + "", + "path to a file with a GitHub API token", + ) } -func buildMainCommand() (*cobra.Command, error) { - repoPath := os.Getenv("ENHANCEMENTS_PATH") - k, err := kepctl.New(repoPath) - if err != nil { - return nil, err - } - - rootCmd := &cobra.Command{ - Use: "kepctl", - Short: "kepctl helps you build keps", - } - - rootCmd.AddCommand(buildCreateCommand(k)) - rootCmd.AddCommand(buildPromoteCommand(k)) - rootCmd.AddCommand(buildQueryCommand(k)) - return rootCmd, nil +func initLogging(*cobra.Command, []string) error { + return log.SetupGlobalLogger(rootOpts.LogLevel) } diff --git a/cmd/kepctl/helper.go b/cmd/kepctl/helper.go deleted file mode 100644 index cec96770a15..00000000000 --- a/cmd/kepctl/helper.go +++ /dev/null @@ -1,31 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package main - -import ( - flag "github.com/spf13/pflag" - - "k8s.io/enhancements/pkg/kepctl" -) - -func addRepoPathFlag( - f *flag.FlagSet, - opts *kepctl.CommonArgs, -) { - f.StringVar(&opts.RepoPath, "repo-path", "", "Path to kubernetes/enhancements") - f.StringVar(&opts.TokenPath, "gh-token-path", "", "Path to a file with a GitHub API token") -} diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index e15ce2fe8fd..2f0b13810d8 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -43,12 +43,16 @@ import ( ) type CommonArgs struct { + // command options + LogLevel string RepoPath string // override the default settings TokenPath string - KEP string // KEP name sig-xxx/xxx-name - Name string - Number string - SIG string + + // KEP options + KEP string // KEP name sig-xxx/xxx-name + Name string + Number string + SIG string } func (c *CommonArgs) validateAndPopulateKEP(args []string) error { From aed343d9a78aa309c7d46c0e3540f1cd3839ae95 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 21:27:36 -0500 Subject: [PATCH 12/19] kepctl: Finish initial refactor Signed-off-by: Stephen Augustus --- cmd/kepctl/cmd/create.go | 103 ++++++++++++++++++++++++++--------- cmd/kepctl/cmd/promote.go | 63 +++++++++++++++------- cmd/kepctl/cmd/query.go | 111 +++++++++++++++++++++++++++++--------- pkg/kepctl/query.go | 2 + 4 files changed, 211 insertions(+), 68 deletions(-) diff --git a/cmd/kepctl/cmd/create.go b/cmd/kepctl/cmd/create.go index 9643e288621..71eb390d89c 100644 --- a/cmd/kepctl/cmd/create.go +++ b/cmd/kepctl/cmd/create.go @@ -17,35 +17,90 @@ limitations under the License. package cmd import ( + "github.com/pkg/errors" "github.com/spf13/cobra" "k8s.io/enhancements/pkg/kepctl" ) -func buildCreateCommand(k *kepctl.Client) *cobra.Command { - opts := kepctl.CreateOpts{} - cmd := &cobra.Command{ - Use: "create [KEP]", - Short: "Create a new KEP", - Long: "Create a new KEP using the current KEP template for the given type", - Example: ` kepctl create sig-architecture/000-mykep`, - PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.Validate(args) - }, - RunE: func(cmd *cobra.Command, args []string) error { - return k.Create(&opts) - }, +// TODO: Struct literal instead? +var createOpts = kepctl.CreateOpts{} + +var createCmd = &cobra.Command{ + Use: "create [KEP]", + Short: "Create a new KEP", + Long: "Create a new KEP using the current KEP template for the given type", + Example: ` kepctl create sig-architecture/000-mykep`, + SilenceUsage: true, + SilenceErrors: true, + PreRunE: func(cmd *cobra.Command, args []string) error { + return createOpts.Validate(args) + }, + RunE: func(*cobra.Command, []string) error { + return runCreate(createOpts) + }, +} + +func init() { + // TODO: Should these all be global args? + createCmd.PersistentFlags().StringVar( + &createOpts.Title, + "title", + "", + "KEP Title", + ) + + createCmd.PersistentFlags().StringArrayVar( + &createOpts.Authors, + "authors", + []string{}, + "Authors", + ) + + createCmd.PersistentFlags().StringArrayVar( + &createOpts.Reviewers, + "reviewers", + []string{}, + "Reviewers", + ) + + createCmd.PersistentFlags().StringVar( + &createOpts.Type, + "type", + "feature", + "KEP Type", + ) + + createCmd.PersistentFlags().StringVarP( + &createOpts.State, + "state", + "s", + "provisional", + "KEP State", + ) + + createCmd.PersistentFlags().StringArrayVar( + &createOpts.SIGS, + "sigs", + []string{}, + "Participating SIGs", + ) + + createCmd.PersistentFlags().StringArrayVar( + &createOpts.PRRApprovers, + "prr-approver", + []string{}, + "PRR Approver", + ) + + rootCmd.AddCommand(createCmd) +} + +func runCreate(createOpts kepctl.CreateOpts) error { + k, err := kepctl.New(createOpts.RepoPath) + if err != nil { + return errors.Wrap(err, "creating kepctl client") } - f := cmd.Flags() - f.StringVar(&opts.Title, "title", "", "KEP Title") - f.StringArrayVar(&opts.Authors, "authors", []string{}, "Authors") - f.StringArrayVar(&opts.Reviewers, "reviewers", []string{}, "Reviewers") - f.StringVar(&opts.Type, "type", "feature", "KEP Type") - f.StringVarP(&opts.State, "state", "s", "provisional", "KEP State") - f.StringArrayVar(&opts.SIGS, "sigs", []string{}, "Participating SIGs") - f.StringArrayVar(&opts.PRRApprovers, "prr-approver", []string{}, "PRR Approver") - - addRepoPathFlag(f, &opts.CommonArgs) - return cmd + return k.Create(&createOpts) } diff --git a/cmd/kepctl/cmd/promote.go b/cmd/kepctl/cmd/promote.go index 99882c91df1..b0be4953dde 100644 --- a/cmd/kepctl/cmd/promote.go +++ b/cmd/kepctl/cmd/promote.go @@ -17,31 +17,56 @@ limitations under the License. package cmd import ( + "github.com/pkg/errors" "github.com/spf13/cobra" "k8s.io/enhancements/pkg/kepctl" ) -func buildPromoteCommand(k *kepctl.Client) *cobra.Command { - opts := kepctl.PromoteOpts{} - cmd := &cobra.Command{ - Use: "promote [KEP]", - Short: "Promote a KEP", - Long: "Promote a KEP to a new stage for a target release", - Example: ` kepctl promote sig-architecture/000-mykep --stage beta --release v1.20`, - PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.Validate(args) - }, - RunE: func(cmd *cobra.Command, args []string) error { - return k.Promote(&opts) - }, - } +// TODO: Struct literal instead? +var promoteOpts = kepctl.PromoteOpts{} + +var promoteCmd = &cobra.Command{ + Use: "promote [KEP]", + Short: "Promote a KEP", + Long: "Promote a KEP to a new stage for a target release", + Example: ` kepctl promote sig-architecture/000-mykep --stage beta --release v1.20`, + SilenceUsage: true, + SilenceErrors: true, + PreRunE: func(cmd *cobra.Command, args []string) error { + return promoteOpts.Validate(args) + }, + RunE: func(*cobra.Command, []string) error { + return runPromote(promoteOpts) + }, +} + +func init() { + // TODO: Should these all be global args? + promoteCmd.PersistentFlags().StringVarP( + &promoteOpts.Stage, + "stage", + "s", + "", + "KEP Stage", + ) - f := cmd.Flags() - f.StringVarP(&opts.Stage, "stage", "s", "", "KEP Stage") - f.StringVarP(&opts.Release, "release", "r", "", "Target Release") + promoteCmd.PersistentFlags().StringVarP( + &promoteOpts.Release, + "release", + "r", + "", + "Target Release", + ) - addRepoPathFlag(f, &opts.CommonArgs) + rootCmd.AddCommand(promoteCmd) +} + +func runPromote(opts kepctl.PromoteOpts) error { + k, err := kepctl.New(opts.RepoPath) + if err != nil { + return errors.Wrap(err, "creating kepctl client") + } - return cmd + return k.Promote(&opts) } diff --git a/cmd/kepctl/cmd/query.go b/cmd/kepctl/cmd/query.go index e651153607d..382fe390a4a 100644 --- a/cmd/kepctl/cmd/query.go +++ b/cmd/kepctl/cmd/query.go @@ -19,37 +19,98 @@ package cmd import ( "fmt" + "github.com/pkg/errors" "github.com/spf13/cobra" "k8s.io/enhancements/pkg/kepctl" ) -func buildQueryCommand(k *kepctl.Client) *cobra.Command { - opts := kepctl.QueryOpts{} - cmd := &cobra.Command{ - Use: "query", - Short: "Query KEPs", - Long: "Query the local filesystem, and optionally GitHub PRs for KEPs", - Example: ` kepctl query --sig architecture --status provisional --include-prs`, - PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.Validate() - }, - RunE: func(cmd *cobra.Command, args []string) error { - return k.Query(&opts) - }, - } +// TODO: Struct literal instead? +var queryOpts = kepctl.QueryOpts{} + +var queryCmd = &cobra.Command{ + Use: "query", + Short: "Query KEPs", + Long: "Query the local filesystem, and optionally GitHub PRs for KEPs", + Example: ` kepctl query --sig architecture --status provisional --include-prs`, + SilenceUsage: true, + SilenceErrors: true, + PreRunE: func(*cobra.Command, []string) error { + return queryOpts.Validate() + }, + RunE: func(*cobra.Command, []string) error { + return runQuery(queryOpts) + }, +} + +func init() { + // TODO: Should these all be global args? + queryCmd.PersistentFlags().StringSliceVar( + &queryOpts.SIG, + "sig", + nil, + "SIG. If not specified, KEPs from all SIGs are shown.", + ) + + queryCmd.PersistentFlags().StringSliceVar( + &queryOpts.Status, + "status", + nil, + "Status", + ) + + queryCmd.PersistentFlags().StringSliceVar( + &queryOpts.Stage, + "stage", + nil, + "Stage", + ) + + queryCmd.PersistentFlags().StringSliceVar( + &queryOpts.PRRApprover, + "prr", + nil, + "Prod Readiness Approver", + ) - f := cmd.Flags() - f.StringSliceVar(&opts.SIG, "sig", nil, "SIG. If not specified, KEPs from all SIGs are shown.") - f.StringSliceVar(&opts.Status, "status", nil, "Status") - f.StringSliceVar(&opts.Stage, "stage", nil, "Stage") - f.StringSliceVar(&opts.PRRApprover, "prr", nil, "Prod Readiness Approver") - f.StringSliceVar(&opts.Approver, "approver", nil, "Approver") - f.StringSliceVar(&opts.Author, "author", nil, "Author") - f.BoolVar(&opts.IncludePRs, "include-prs", false, "Include PRs in the results") - f.StringVar(&opts.Output, "output", kepctl.DefaultOutputOpt, fmt.Sprintf("Output format. Can be %v", kepctl.SupportedOutputOpts)) + queryCmd.PersistentFlags().StringSliceVar( + &queryOpts.Approver, + "approver", + nil, + "Approver", + ) - addRepoPathFlag(f, &opts.CommonArgs) + queryCmd.PersistentFlags().StringSliceVar( + &queryOpts.Author, + "author", + nil, + "Author", + ) + + queryCmd.PersistentFlags().BoolVar( + &queryOpts.IncludePRs, + "include-prs", + false, + "Include PRs in the results", + ) + + queryCmd.PersistentFlags().StringVar( + &queryOpts.Output, + "output", + kepctl.DefaultOutputOpt, + fmt.Sprintf( + "Output format. Can be %v", kepctl.SupportedOutputOpts, + ), + ) + + rootCmd.AddCommand(queryCmd) +} + +func runQuery(queryOpts kepctl.QueryOpts) error { + k, err := kepctl.New(queryOpts.RepoPath) + if err != nil { + return errors.Wrap(err, "creating kepctl client") + } - return cmd + return k.Query(&queryOpts) } diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index dbbad500738..d6a56aa028b 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -63,9 +63,11 @@ func (c *QueryOpts) Validate() error { if err != nil { return err } + if len(sigs) == 0 { return fmt.Errorf("no SIG matches any of the passed regular expressions") } + c.SIG = sigs } else { // if no SIGs are passed, list KEPs from all SIGs From 322997e6bb22128f2ef0f907005b8a56be7a03bf Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sat, 30 Jan 2021 22:46:40 -0500 Subject: [PATCH 13/19] test: Init with KEP metadata test from cmd/kepval Signed-off-by: Stephen Augustus --- hack/verify-kep-metadata.sh | 2 +- test/OWNERS | 9 +++++++++ cmd/kepval/main_test.go => test/metadata_test.go | 6 +++--- 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 test/OWNERS rename cmd/kepval/main_test.go => test/metadata_test.go (98%) diff --git a/hack/verify-kep-metadata.sh b/hack/verify-kep-metadata.sh index 425ac8b81e7..23e4932800b 100755 --- a/hack/verify-kep-metadata.sh +++ b/hack/verify-kep-metadata.sh @@ -23,4 +23,4 @@ ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" cd "${ROOT}" # run the tests -GO111MODULE=on go test ./cmd/kepval/... +GO111MODULE=on go test ./test/metadata_test.go diff --git a/test/OWNERS b/test/OWNERS new file mode 100644 index 00000000000..3eca1e29d9c --- /dev/null +++ b/test/OWNERS @@ -0,0 +1,9 @@ +# See the OWNERS docs at https://go.k8s.io/owners + +approvers: + - kep-tools-approvers +reviewers: + - kep-tools-reviewers + +labels: + - area/enhancements diff --git a/cmd/kepval/main_test.go b/test/metadata_test.go similarity index 98% rename from cmd/kepval/main_test.go rename to test/metadata_test.go index a9ad5cc17dd..3424aa4e792 100644 --- a/cmd/kepval/main_test.go +++ b/test/metadata_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package main +package test import ( "os" @@ -37,7 +37,7 @@ func TestValidation(t *testing.T) { // Find all the keps files := []string{} err := filepath.Walk( - filepath.Join("..", "..", kepsDir), + filepath.Join("..", kepsDir), func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -148,7 +148,7 @@ func TestValidation(t *testing.T) { // TODO: Is this a duplicate of the package function? // ignore certain files in the keps/ subdirectory func ignore(dir, name string) bool { - if dir == "../../keps/NNNN-kep-template" { + if dir == "../keps/NNNN-kep-template" { return true // ignore the template directory because its metadata file does not use a valid sig name } From 0e787303c504ec325705460dd7b08dcac401240f Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sun, 31 Jan 2021 21:13:38 -0500 Subject: [PATCH 14/19] api: Add `Document` interface and PRR approval types Signed-off-by: Stephen Augustus --- api/approval.go | 37 ++++++++++++++++++++++++++++++++++++ api/document.go | 21 ++++++++++++++++++++ pkg/kepval/prrs/approvals.go | 24 +++-------------------- 3 files changed, 61 insertions(+), 21 deletions(-) create mode 100644 api/approval.go create mode 100644 api/document.go diff --git a/api/approval.go b/api/approval.go new file mode 100644 index 00000000000..0d139c43b48 --- /dev/null +++ b/api/approval.go @@ -0,0 +1,37 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +type PRRApprovals []*PRRApproval + +func (p *PRRApprovals) AddPRRApproval(prrApproval *PRRApproval) { + *p = append(*p, prrApproval) +} + +type PRRApproval struct { + Number string `json:"kep-number" yaml:"kep-number"` + Alpha PRRMilestone `json:"alpha" yaml:"alpha,omitempty"` + Beta PRRMilestone `json:"beta" yaml:"beta,omitempty"` + Stable PRRMilestone `json:"stable" yaml:"stable,omitempty"` + + Error error `json:"-" yaml:"-"` +} + +// TODO(api): Can we refactor the proposal `Milestone` to retrieve this? +type PRRMilestone struct { + Approver string `json:"approver" yaml:"approver"` +} diff --git a/api/document.go b/api/document.go new file mode 100644 index 00000000000..b1d5934b7b4 --- /dev/null +++ b/api/document.go @@ -0,0 +1,21 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +// TODO(api): Populate interface +// TODO(api): Mock interface +type Document interface{} diff --git a/pkg/kepval/prrs/approvals.go b/pkg/kepval/prrs/approvals.go index 61348515636..240cb5ee6b8 100644 --- a/pkg/kepval/prrs/approvals.go +++ b/pkg/kepval/prrs/approvals.go @@ -24,31 +24,13 @@ import ( "github.com/pkg/errors" "gopkg.in/yaml.v3" + "k8s.io/enhancements/api" "k8s.io/enhancements/pkg/kepval/prrs/validations" ) -type Approvals []*Approval - -func (a *Approvals) AddApproval(approval *Approval) { - *a = append(*a, approval) -} - -type Milestone struct { - Approver string `json:"approver" yaml:"approver"` -} - -type Approval struct { - Number string `json:"kep-number" yaml:"kep-number"` - Alpha Milestone `json:"alpha" yaml:"alpha"` - Beta Milestone `json:"beta" yaml:"beta"` - Stable Milestone `json:"stable" yaml:"stable"` - - Error error `json:"-" yaml:"-"` -} - type Parser struct{} -func (p *Parser) Parse(in io.Reader) *Approval { +func (p *Parser) Parse(in io.Reader) *api.PRRApproval { scanner := bufio.NewScanner(in) var body bytes.Buffer for scanner.Scan() { @@ -56,7 +38,7 @@ func (p *Parser) Parse(in io.Reader) *Approval { body.WriteString(line) } - approval := &Approval{} + approval := &api.PRRApproval{} if err := scanner.Err(); err != nil { approval.Error = errors.Wrap(err, "error reading file") return approval From eb2dba9a2ab80d790278054bc4fbe675973cf732 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sun, 31 Jan 2021 21:40:09 -0500 Subject: [PATCH 15/19] pkg: Move `kepval` packages to `legacy` Signed-off-by: Stephen Augustus --- pkg/{kepval => legacy}/keps/proposals.go | 3 ++- pkg/{kepval => legacy}/keps/proposals_test.go | 0 pkg/{kepval => legacy}/keps/validations/yaml.go | 0 pkg/{kepval => legacy}/keps/validations/yaml_test.go | 0 pkg/{kepval => legacy}/prrs/approvals.go | 0 pkg/{kepval => legacy}/prrs/approvals_test.go | 0 pkg/{kepval => legacy}/prrs/validations/yaml.go | 0 pkg/{kepval => legacy}/prrs/validations/yaml_test.go | 0 pkg/{kepval => legacy}/util/errors.go | 0 pkg/{kepval => legacy}/util/helpers.go | 0 10 files changed, 2 insertions(+), 1 deletion(-) rename pkg/{kepval => legacy}/keps/proposals.go (97%) rename pkg/{kepval => legacy}/keps/proposals_test.go (100%) rename pkg/{kepval => legacy}/keps/validations/yaml.go (100%) rename pkg/{kepval => legacy}/keps/validations/yaml_test.go (100%) rename pkg/{kepval => legacy}/prrs/approvals.go (100%) rename pkg/{kepval => legacy}/prrs/approvals_test.go (100%) rename pkg/{kepval => legacy}/prrs/validations/yaml.go (100%) rename pkg/{kepval => legacy}/prrs/validations/yaml_test.go (100%) rename pkg/{kepval => legacy}/util/errors.go (100%) rename pkg/{kepval => legacy}/util/helpers.go (100%) diff --git a/pkg/kepval/keps/proposals.go b/pkg/legacy/keps/proposals.go similarity index 97% rename from pkg/kepval/keps/proposals.go rename to pkg/legacy/keps/proposals.go index e898263bd2c..6b4e9ad38b1 100644 --- a/pkg/kepval/keps/proposals.go +++ b/pkg/legacy/keps/proposals.go @@ -28,7 +28,7 @@ import ( "gopkg.in/yaml.v3" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/kepval/keps/validations" + "k8s.io/enhancements/pkg/legacy/keps/validations" ) type Parser struct{} @@ -70,6 +70,7 @@ func (p *Parser) Parse(in io.Reader) *api.Proposal { proposal.Error = errors.Wrap(err, "error unmarshaling YAML") return proposal } + if err := validations.ValidateStructure(test); err != nil { proposal.Error = errors.Wrap(err, "error validating KEP metadata") return proposal diff --git a/pkg/kepval/keps/proposals_test.go b/pkg/legacy/keps/proposals_test.go similarity index 100% rename from pkg/kepval/keps/proposals_test.go rename to pkg/legacy/keps/proposals_test.go diff --git a/pkg/kepval/keps/validations/yaml.go b/pkg/legacy/keps/validations/yaml.go similarity index 100% rename from pkg/kepval/keps/validations/yaml.go rename to pkg/legacy/keps/validations/yaml.go diff --git a/pkg/kepval/keps/validations/yaml_test.go b/pkg/legacy/keps/validations/yaml_test.go similarity index 100% rename from pkg/kepval/keps/validations/yaml_test.go rename to pkg/legacy/keps/validations/yaml_test.go diff --git a/pkg/kepval/prrs/approvals.go b/pkg/legacy/prrs/approvals.go similarity index 100% rename from pkg/kepval/prrs/approvals.go rename to pkg/legacy/prrs/approvals.go diff --git a/pkg/kepval/prrs/approvals_test.go b/pkg/legacy/prrs/approvals_test.go similarity index 100% rename from pkg/kepval/prrs/approvals_test.go rename to pkg/legacy/prrs/approvals_test.go diff --git a/pkg/kepval/prrs/validations/yaml.go b/pkg/legacy/prrs/validations/yaml.go similarity index 100% rename from pkg/kepval/prrs/validations/yaml.go rename to pkg/legacy/prrs/validations/yaml.go diff --git a/pkg/kepval/prrs/validations/yaml_test.go b/pkg/legacy/prrs/validations/yaml_test.go similarity index 100% rename from pkg/kepval/prrs/validations/yaml_test.go rename to pkg/legacy/prrs/validations/yaml_test.go diff --git a/pkg/kepval/util/errors.go b/pkg/legacy/util/errors.go similarity index 100% rename from pkg/kepval/util/errors.go rename to pkg/legacy/util/errors.go diff --git a/pkg/kepval/util/helpers.go b/pkg/legacy/util/helpers.go similarity index 100% rename from pkg/kepval/util/helpers.go rename to pkg/legacy/util/helpers.go From ea111a035d0edaec2f5e064fb65b1836dec85156 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sun, 31 Jan 2021 21:49:01 -0500 Subject: [PATCH 16/19] pkg: Replace `kepval/**` imports with `legacy` packages Signed-off-by: Stephen Augustus --- api/approval.go | 13 +++++++++++++ cmd/kepify/main.go | 2 +- pkg/kepctl/kepctl.go | 6 +++--- pkg/kepctl/query.go | 2 +- pkg/legacy/keps/proposals_test.go | 2 +- pkg/legacy/keps/validations/yaml.go | 2 +- pkg/legacy/prrs/approvals.go | 14 +------------- pkg/legacy/prrs/validations/yaml.go | 2 +- test/metadata_test.go | 4 ++-- 9 files changed, 24 insertions(+), 23 deletions(-) diff --git a/api/approval.go b/api/approval.go index 0d139c43b48..6106707eed1 100644 --- a/api/approval.go +++ b/api/approval.go @@ -31,6 +31,19 @@ type PRRApproval struct { Error error `json:"-" yaml:"-"` } +func (p *PRRApproval) ApproverForStage(stage string) string { + switch stage { + case "alpha": + return p.Alpha.Approver + case "beta": + return p.Beta.Approver + case "stable": + return p.Stable.Approver + } + + return "" +} + // TODO(api): Can we refactor the proposal `Milestone` to retrieve this? type PRRMilestone struct { Approver string `json:"approver" yaml:"approver"` diff --git a/cmd/kepify/main.go b/cmd/kepify/main.go index 93887239a7a..7acf8f0907a 100644 --- a/cmd/kepify/main.go +++ b/cmd/kepify/main.go @@ -26,7 +26,7 @@ import ( "strings" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/kepval/keps" + "k8s.io/enhancements/pkg/legacy/keps" ) func Usage() { diff --git a/pkg/kepctl/kepctl.go b/pkg/kepctl/kepctl.go index 2f0b13810d8..03833d506b7 100644 --- a/pkg/kepctl/kepctl.go +++ b/pkg/kepctl/kepctl.go @@ -37,8 +37,8 @@ import ( "gopkg.in/yaml.v3" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/kepval/keps" - "k8s.io/enhancements/pkg/kepval/prrs" + "k8s.io/enhancements/pkg/legacy/keps" + "k8s.io/enhancements/pkg/legacy/prrs" "k8s.io/test-infra/prow/git" ) @@ -182,7 +182,7 @@ func findLocalKEPMeta(repoPath, sig string) ([]string, error) { "keps", sig) - // TODO(lint): importShadow: shadow of imported from 'k8s.io/enhancements/pkg/kepval/keps' package 'keps' (gocritic) + // TODO(lint): importShadow: shadow of imported from 'k8s.io/enhancements/pkg/legacy/keps' package 'keps' (gocritic) //nolint:gocritic keps := []string{} diff --git a/pkg/kepctl/query.go b/pkg/kepctl/query.go index d6a56aa028b..fdce725be7e 100644 --- a/pkg/kepctl/query.go +++ b/pkg/kepctl/query.go @@ -23,7 +23,7 @@ import ( "github.com/pkg/errors" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/kepval/util" + "k8s.io/enhancements/pkg/legacy/util" ) var ( diff --git a/pkg/legacy/keps/proposals_test.go b/pkg/legacy/keps/proposals_test.go index 2907bbb4f13..6f1bc36292a 100644 --- a/pkg/legacy/keps/proposals_test.go +++ b/pkg/legacy/keps/proposals_test.go @@ -20,7 +20,7 @@ import ( "strings" "testing" - "k8s.io/enhancements/pkg/kepval/keps" + "k8s.io/enhancements/pkg/legacy/keps" ) func TestValidParsing(t *testing.T) { diff --git a/pkg/legacy/keps/validations/yaml.go b/pkg/legacy/keps/validations/yaml.go index ae63f74e326..15918782b73 100644 --- a/pkg/legacy/keps/validations/yaml.go +++ b/pkg/legacy/keps/validations/yaml.go @@ -21,7 +21,7 @@ import ( "sort" "strings" - "k8s.io/enhancements/pkg/kepval/util" + "k8s.io/enhancements/pkg/legacy/util" ) var ( diff --git a/pkg/legacy/prrs/approvals.go b/pkg/legacy/prrs/approvals.go index 240cb5ee6b8..eb3fc62e65d 100644 --- a/pkg/legacy/prrs/approvals.go +++ b/pkg/legacy/prrs/approvals.go @@ -25,7 +25,7 @@ import ( "gopkg.in/yaml.v3" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/kepval/prrs/validations" + "k8s.io/enhancements/pkg/legacy/prrs/validations" ) type Parser struct{} @@ -59,15 +59,3 @@ func (p *Parser) Parse(in io.Reader) *api.PRRApproval { return approval } - -func (a *Approval) ApproverForStage(stage string) string { - switch stage { - case "alpha": - return a.Alpha.Approver - case "beta": - return a.Beta.Approver - case "stable": - return a.Stable.Approver - } - return "" -} diff --git a/pkg/legacy/prrs/validations/yaml.go b/pkg/legacy/prrs/validations/yaml.go index 1e85912e5b2..40a993b312c 100644 --- a/pkg/legacy/prrs/validations/yaml.go +++ b/pkg/legacy/prrs/validations/yaml.go @@ -21,7 +21,7 @@ import ( "sort" "strings" - "k8s.io/enhancements/pkg/kepval/util" + "k8s.io/enhancements/pkg/legacy/util" ) var mandatoryKeys = []string{"kep-number"} diff --git a/test/metadata_test.go b/test/metadata_test.go index 3424aa4e792..0411fd948d8 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -22,8 +22,8 @@ import ( "strings" "testing" - "k8s.io/enhancements/pkg/kepval/keps" - "k8s.io/enhancements/pkg/kepval/prrs" + "k8s.io/enhancements/pkg/legacy/keps" + "k8s.io/enhancements/pkg/legacy/prrs" ) const ( From 7fb91eb5cf2250e14fae7bbe2a4e4e92da428943 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sun, 31 Jan 2021 21:53:00 -0500 Subject: [PATCH 17/19] test: Fixup PRR approval path Signed-off-by: Stephen Augustus --- test/metadata_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index 0411fd948d8..a463fda05de 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -75,7 +75,7 @@ func TestValidation(t *testing.T) { kepParser := &keps.Parser{} prrParser := &prrs.Parser{} - prrsDir := filepath.Join("..", "..", prrsDir) + prrsDir := filepath.Join("..", prrsDir) for _, filename := range files { t.Run(filename, func(t *testing.T) { From 8ffe0d293dd39b67a05694b81f9f61bd4c6d0b11 Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sun, 31 Jan 2021 23:46:57 -0500 Subject: [PATCH 18/19] api: Validate PRRs by field Signed-off-by: Stephen Augustus --- api/approval.go | 59 +++++++++++++++++++++++++++++++++--- api/document.go | 22 +++++++++++++- go.mod | 3 +- go.sum | 11 +++++++ pkg/legacy/keps/proposals.go | 11 ++++--- test/metadata_test.go | 14 +++++++-- 6 files changed, 105 insertions(+), 15 deletions(-) diff --git a/api/approval.go b/api/approval.go index 6106707eed1..26521a26ad9 100644 --- a/api/approval.go +++ b/api/approval.go @@ -16,6 +16,16 @@ limitations under the License. package api +import ( + "bufio" + "bytes" + "io" + + "github.com/go-playground/validator/v10" + "github.com/pkg/errors" + "gopkg.in/yaml.v3" +) + type PRRApprovals []*PRRApproval func (p *PRRApprovals) AddPRRApproval(prrApproval *PRRApproval) { @@ -28,17 +38,27 @@ type PRRApproval struct { Beta PRRMilestone `json:"beta" yaml:"beta,omitempty"` Stable PRRMilestone `json:"stable" yaml:"stable,omitempty"` + // TODO(api): Move to separate struct for handling document parsing Error error `json:"-" yaml:"-"` } -func (p *PRRApproval) ApproverForStage(stage string) string { +func (prr *PRRApproval) Validate() error { + v := validator.New() + if err := v.Struct(prr); err != nil { + return errors.Wrap(err, "running validation") + } + + return nil +} + +func (prr *PRRApproval) ApproverForStage(stage string) string { switch stage { case "alpha": - return p.Alpha.Approver + return prr.Alpha.Approver case "beta": - return p.Beta.Approver + return prr.Beta.Approver case "stable": - return p.Stable.Approver + return prr.Stable.Approver } return "" @@ -46,5 +66,34 @@ func (p *PRRApproval) ApproverForStage(stage string) string { // TODO(api): Can we refactor the proposal `Milestone` to retrieve this? type PRRMilestone struct { - Approver string `json:"approver" yaml:"approver"` + Approver string `json:"approver" yaml:"approver" validate:"required"` +} + +type PRRHandler Parser + +// TODO(api): Make this a generic parser for all `Document` types +func (p *PRRHandler) Parse(in io.Reader) (*PRRApproval, error) { + scanner := bufio.NewScanner(in) + var body bytes.Buffer + for scanner.Scan() { + line := scanner.Text() + "\n" + body.WriteString(line) + } + + approval := &PRRApproval{} + if err := scanner.Err(); err != nil { + return approval, errors.Wrap(err, "reading file") + } + + if err := yaml.Unmarshal(body.Bytes(), &approval); err != nil { + p.Errors = append(p.Errors, errors.Wrap(err, "error unmarshalling YAML")) + return approval, errors.Wrap(err, "unmarshalling YAML") + } + + if valErr := approval.Validate(); valErr != nil { + p.Errors = append(p.Errors, errors.Wrap(valErr, "validating PRR")) + return approval, errors.Wrap(valErr, "validating PRR") + } + + return approval, nil } diff --git a/api/document.go b/api/document.go index b1d5934b7b4..99317ff1a8e 100644 --- a/api/document.go +++ b/api/document.go @@ -16,6 +16,26 @@ limitations under the License. package api +import ( + "io" +) + // TODO(api): Populate interface // TODO(api): Mock interface -type Document interface{} +type File interface { + Parse(io.Reader) (Document, error) +} + +// TODO(api): Populate interface +// TODO(api): Mock interface +// Document is an interface satisfied by the following types: +// - `Proposal` (KEP) +// - `PRRApproval` +// - `Receipt` (coming soon) +type Document interface { + Validate() error +} + +type Parser struct { + Errors []error +} diff --git a/go.mod b/go.mod index 69cf55a0080..a7fbcc02be6 100644 --- a/go.mod +++ b/go.mod @@ -3,14 +3,15 @@ module k8s.io/enhancements go 1.15 require ( + github.com/go-playground/validator/v10 v10.4.1 github.com/google/go-github/v32 v32.1.0 + github.com/leodido/go-urn v1.2.1 // indirect github.com/maxbrunsfeld/counterfeiter/v6 v6.3.0 github.com/olekukonko/tablewriter v0.0.4 github.com/pkg/errors v0.9.1 github.com/psampaz/go-mod-outdated v0.7.0 github.com/sirupsen/logrus v1.7.0 github.com/spf13/cobra v1.1.1 - github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 golang.org/x/oauth2 v0.0.0-20210112200429-01de73cf58bd gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c diff --git a/go.sum b/go.sum index 4002b3da24e..fd5a174a0fd 100644 --- a/go.sum +++ b/go.sum @@ -482,6 +482,14 @@ github.com/go-openapi/validate v0.17.0/go.mod h1:Uh4HdOzKt19xGIGm1qHf/ofbX1YQ4Y+ github.com/go-openapi/validate v0.18.0/go.mod h1:Uh4HdOzKt19xGIGm1qHf/ofbX1YQ4Y+MYsct2VUrAJ4= github.com/go-openapi/validate v0.19.2/go.mod h1:1tRCw7m3jtI8eNWEEliiAqUIcBztB2KDnRCRMUi7GTA= github.com/go-openapi/validate v0.19.5/go.mod h1:8DJv2CVJQ6kGNpFW6eV9N3JviE1C85nY1c2z52x1Gk4= +github.com/go-playground/assert/v2 v2.0.1 h1:MsBgLAaY856+nPRTKrp3/OZK38U/wa0CcBYNjji3q3A= +github.com/go-playground/assert/v2 v2.0.1/go.mod h1:VDjEfimB/XKnb+ZQfWdccd7VUvScMdVu0Titje2rxJ4= +github.com/go-playground/locales v0.13.0 h1:HyWk6mgj5qFqCT5fjGBuRArbVDfE4hi8+e8ceBS/t7Q= +github.com/go-playground/locales v0.13.0/go.mod h1:taPMhCMXrRLJO55olJkUXHZBHCxTMfnGwq/HNwmWNS8= +github.com/go-playground/universal-translator v0.17.0 h1:icxd5fm+REJzpZx7ZfpaD876Lmtgy7VtROAbHHXk8no= +github.com/go-playground/universal-translator v0.17.0/go.mod h1:UkSxE5sNxxRwHyU+Scu5vgOQjsIJAF8j9muTVoKLVtA= +github.com/go-playground/validator/v10 v10.4.1 h1:pH2c5ADXtd66mxoE0Zm9SUhxE20r7aM3F26W0hOn+GE= +github.com/go-playground/validator/v10 v10.4.1/go.mod h1:nlOn6nFhuKACm19sB/8EGNn9GlaMV7XkbRSipzJ0Ii4= github.com/go-sql-driver/mysql v0.0.0-20160411075031-7ebe0a500653/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w= github.com/go-sql-driver/mysql v1.4.0/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w= github.com/go-sql-driver/mysql v1.4.1/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w= @@ -822,6 +830,9 @@ github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII= +github.com/leodido/go-urn v1.2.1 h1:BqpAaACuzVSgi/VLzGZIobT2z4v53pjosyNd9Yv6n/w= +github.com/leodido/go-urn v1.2.1/go.mod h1:zt4jvISO2HfUBqxjfIshjdMTYS56ZS/qv49ictyFfxY= github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/lib/pq v1.1.1/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/lib/pq v1.2.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= diff --git a/pkg/legacy/keps/proposals.go b/pkg/legacy/keps/proposals.go index 6b4e9ad38b1..68ca4449384 100644 --- a/pkg/legacy/keps/proposals.go +++ b/pkg/legacy/keps/proposals.go @@ -28,7 +28,6 @@ import ( "gopkg.in/yaml.v3" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/legacy/keps/validations" ) type Parser struct{} @@ -71,10 +70,12 @@ func (p *Parser) Parse(in io.Reader) *api.Proposal { return proposal } - if err := validations.ValidateStructure(test); err != nil { - proposal.Error = errors.Wrap(err, "error validating KEP metadata") - return proposal - } + /* + if err := validations.ValidateStructure(test); err != nil { + proposal.Error = errors.Wrap(err, "error validating KEP metadata") + return proposal + } + */ proposal.Error = yaml.Unmarshal(metadata, proposal) proposal.ID = hash(proposal.OwningSIG + ":" + proposal.Title) diff --git a/test/metadata_test.go b/test/metadata_test.go index a463fda05de..92ea8b7c56e 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -22,8 +22,10 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" + + "k8s.io/enhancements/api" "k8s.io/enhancements/pkg/legacy/keps" - "k8s.io/enhancements/pkg/legacy/prrs" ) const ( @@ -74,7 +76,7 @@ func TestValidation(t *testing.T) { } kepParser := &keps.Parser{} - prrParser := &prrs.Parser{} + prrHandler := &api.PRRHandler{} prrsDir := filepath.Join("..", prrsDir) for _, filename := range files { @@ -83,6 +85,7 @@ func TestValidation(t *testing.T) { if err != nil { t.Fatalf("could not open file %s: %v\n", filename, err) } + defer kepFile.Close() kep := kepParser.Parse(kepFile) @@ -114,10 +117,14 @@ func TestValidation(t *testing.T) { t.Errorf("To get PRR approval modify appropriately file %s and have this approved by PRR team", prrFilename) return } + if err != nil { t.Fatalf("could not open file %s: %v\n", prrFilename, err) } - prr := prrParser.Parse(prrFile) + + prr, prrParseErr := prrHandler.Parse(prrFile) + require.Nil(t, prrParseErr) + if prr.Error != nil { t.Errorf("PRR approval file %v has an error: %v", prrFilename, prr.Error) return @@ -132,6 +139,7 @@ func TestValidation(t *testing.T) { case "stable": stagePRRApprover = prr.Stable.Approver } + if len(stageMilestone) > 0 && stageMilestone >= "v1.21" { // PRR approval is needed. if stagePRRApprover == "" { From 685eecf8f1f37bf166121b29461e1ceeb4aacc7c Mon Sep 17 00:00:00 2001 From: Stephen Augustus Date: Sun, 31 Jan 2021 23:50:30 -0500 Subject: [PATCH 19/19] api: Validate KEPs by field Signed-off-by: Stephen Augustus --- api/approval.go | 2 +- api/proposal.go | 76 +++++++++++++++++++++++++++++++++++++++++++ test/metadata_test.go | 68 +++++++++++++++++++++----------------- 3 files changed, 115 insertions(+), 31 deletions(-) diff --git a/api/approval.go b/api/approval.go index 26521a26ad9..5f53bc54a82 100644 --- a/api/approval.go +++ b/api/approval.go @@ -66,7 +66,7 @@ func (prr *PRRApproval) ApproverForStage(stage string) string { // TODO(api): Can we refactor the proposal `Milestone` to retrieve this? type PRRMilestone struct { - Approver string `json:"approver" yaml:"approver" validate:"required"` + Approver string `json:"approver" yaml:"approver"` } type PRRHandler Parser diff --git a/api/proposal.go b/api/proposal.go index 1b89319a8dc..84ccdbfe819 100644 --- a/api/proposal.go +++ b/api/proposal.go @@ -16,6 +16,19 @@ limitations under the License. package api +import ( + "bufio" + "bytes" + "crypto/md5" + "fmt" + "io" + "strings" + + "github.com/go-playground/validator/v10" + "github.com/pkg/errors" + "gopkg.in/yaml.v3" +) + type Proposals []*Proposal func (p *Proposals) AddProposal(proposal *Proposal) { @@ -56,6 +69,65 @@ type Proposal struct { Contents string `json:"markdown" yaml:"-"` } +func (p *Proposal) Validate() error { + v := validator.New() + if err := v.Struct(p); err != nil { + return errors.Wrap(err, "running validation") + } + + return nil +} + +type KEPHandler Parser + +// TODO(api): Make this a generic parser for all `Document` types +func (k *KEPHandler) Parse(in io.Reader) (*Proposal, error) { + scanner := bufio.NewScanner(in) + count := 0 + metadata := []byte{} + var body bytes.Buffer + for scanner.Scan() { + line := scanner.Text() + "\n" + if strings.Contains(line, "---") { + count++ + continue + } + if count == 1 { + metadata = append(metadata, []byte(line)...) + } else { + body.WriteString(line) + } + } + + kep := &Proposal{ + Contents: body.String(), + } + + if err := scanner.Err(); err != nil { + return kep, errors.Wrap(err, "reading file") + } + + // this file is just the KEP metadata + if count == 0 { + metadata = body.Bytes() + kep.Contents = "" + } + + if err := yaml.Unmarshal(metadata, &kep); err != nil { + k.Errors = append(k.Errors, errors.Wrap(err, "error unmarshalling YAML")) + return kep, errors.Wrap(err, "unmarshalling YAML") + } + + if valErr := kep.Validate(); valErr != nil { + k.Errors = append(k.Errors, errors.Wrap(valErr, "validating KEP")) + return kep, errors.Wrap(valErr, "validating KEP") + } + + kep.ID = hash(kep.OwningSIG + ":" + kep.Title) + + return kep, nil +} + type Milestone struct { Alpha string `json:"alpha" yaml:"alpha"` Beta string `json:"beta" yaml:"beta"` @@ -66,3 +138,7 @@ type FeatureGate struct { Name string `json:"name" yaml:"name"` Components []string `json:"components" yaml:"components"` } + +func hash(s string) string { + return fmt.Sprintf("%x", md5.Sum([]byte(s))) +} diff --git a/test/metadata_test.go b/test/metadata_test.go index 92ea8b7c56e..d0532b5b502 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -25,7 +25,6 @@ import ( "github.com/stretchr/testify/require" "k8s.io/enhancements/api" - "k8s.io/enhancements/pkg/legacy/keps" ) const ( @@ -34,37 +33,14 @@ const ( kepMetadata = "kep.yaml" ) -// This is the actual validation check of all keps in this repo +var files = []string{} + +// This is the actual validation check of all KEPs in this repo func TestValidation(t *testing.T) { // Find all the keps - files := []string{} err := filepath.Walk( filepath.Join("..", kepsDir), - func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.IsDir() { - return nil - } - - dir := filepath.Dir(path) - // true if the file is a symlink - if info.Mode()&os.ModeSymlink != 0 { - // assume symlink from old KEP location to new - newLocation, err := os.Readlink(path) - if err != nil { - return err - } - files = append(files, filepath.Join(dir, filepath.Dir(newLocation), kepMetadata)) - return nil - } - if ignore(dir, info.Name()) { - return nil - } - files = append(files, path) - return nil - }, + walkFn, ) // This indicates a problem walking the filepath, not a validation error. if err != nil { @@ -75,7 +51,7 @@ func TestValidation(t *testing.T) { t.Fatal("must find more than 0 keps") } - kepParser := &keps.Parser{} + kepHandler := &api.KEPHandler{} prrHandler := &api.PRRHandler{} prrsDir := filepath.Join("..", prrsDir) @@ -88,7 +64,9 @@ func TestValidation(t *testing.T) { defer kepFile.Close() - kep := kepParser.Parse(kepFile) + kep, kepParseErr := kepHandler.Parse(kepFile) + require.Nil(t, kepParseErr) + if kep.Error != nil { t.Errorf("%v has an error: %v", filename, kep.Error) } @@ -152,6 +130,36 @@ func TestValidation(t *testing.T) { } } +var walkFn = func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if info.IsDir() { + return nil + } + + dir := filepath.Dir(path) + // true if the file is a symlink + if info.Mode()&os.ModeSymlink != 0 { + // assume symlink from old KEP location to new + newLocation, err := os.Readlink(path) + if err != nil { + return err + } + + files = append(files, filepath.Join(dir, filepath.Dir(newLocation), kepMetadata)) + return nil + } + + if ignore(dir, info.Name()) { + return nil + } + + files = append(files, path) + return nil +} + // TODO: Consider replacing with a .kepignore file // TODO: Is this a duplicate of the package function? // ignore certain files in the keps/ subdirectory