diff --git a/enhancements/single-node/single-node-openshift-with-workers.md b/enhancements/single-node/single-node-openshift-with-workers.md index 9e7b862b42..eafd538313 100644 --- a/enhancements/single-node/single-node-openshift-with-workers.md +++ b/enhancements/single-node/single-node-openshift-with-workers.md @@ -7,12 +7,12 @@ reviewers: - "@eranco74" - "@tsorya" - "@dhellmann" - - "@Miciah" - edge networking + - "@Miciah - edge networking" - "@bparees" - - "@JoelSpeed" - API - - "@staebler" - installer + - "@JoelSpeed - API" + - "@staebler - installer" - "@derekwaynecarr" - - "@cgwalters" - MCO + - "@cgwalters - MCO" approvers: - "@dhellmann" api-approvers: # in case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers) diff --git a/guidelines/enhancement_template.md b/guidelines/enhancement_template.md index 3c2b53f406..fafccd6899 100644 --- a/guidelines/enhancement_template.md +++ b/guidelines/enhancement_template.md @@ -1,14 +1,12 @@ --- title: neat-enhancement-idea authors: - - "@janedoe" -reviewers: # Include a comment about what domain expertise a reviewer is expected to bring. For example, "- @networkguru, for networking aspects" - TBD - - "@alicedoe" +reviewers: # Include a comment about what domain expertise a reviewer is expected to bring and what area of the enhancement you expect them to focus on. For example: - "@networkguru, for networking aspects, please look at IP bootstrapping aspect" + - TBD approvers: - TBD - - "@oscardoe" -api-approvers: # in case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers) +api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" - TBD creation-date: yyyy-mm-dd last-updated: yyyy-mm-dd diff --git a/hack/Dockerfile.markdownlint b/hack/Dockerfile.markdownlint index a14b2e9315..03be38453e 100644 --- a/hack/Dockerfile.markdownlint +++ b/hack/Dockerfile.markdownlint @@ -1,6 +1,6 @@ FROM fedora WORKDIR /workdir COPY install-markdownlint.sh /tmp -RUN dnf install -y git +RUN dnf install -y git golang RUN /tmp/install-markdownlint.sh ENTRYPOINT /workdir/hack/markdownlint.sh diff --git a/hack/markdownlint.sh b/hack/markdownlint.sh index 0c9dfe9ec6..3023914c8f 100755 --- a/hack/markdownlint.sh +++ b/hack/markdownlint.sh @@ -3,3 +3,5 @@ markdownlint-cli2 '**/*.md' $(dirname $0)/template-lint.sh + +$(dirname $0)/metadata-lint.sh diff --git a/hack/metadata-lint.sh b/hack/metadata-lint.sh new file mode 100755 index 0000000000..5464ffec47 --- /dev/null +++ b/hack/metadata-lint.sh @@ -0,0 +1,20 @@ +#!/bin/bash + +set -x + +# Ensure the enhancement metadata includes the required information + +set -o errexit +set -o nounset +set -o pipefail + +# We only look at the files that have changed in the current PR, to +# avoid problems when the template is changed in a way that is +# incompatible with existing documents. +CHANGED_FILES=$(git log --name-only --pretty= "${PULL_BASE_SHA:-origin/master}.." -- \ + | (grep '^enhancements.*\.md$' || true) \ + | sort -u) + +(cd tools && go build -o ../enhancement-tool -mod=mod ./main.go) + +./enhancement-tool metadata-lint ${CHANGED_FILES} diff --git a/tools/cmd/annualSummary.go b/tools/cmd/annualSummary.go index ef48e26f4b..596037d623 100644 --- a/tools/cmd/annualSummary.go +++ b/tools/cmd/annualSummary.go @@ -41,6 +41,8 @@ var annualSummaryCmd = &cobra.Command{ Short: "Summarize the enhancements work over the previous year", // Long: ``, RunE: func(cmd *cobra.Command, args []string) error { + initConfig() + earliestDate := time.Date(year, time.January, 1, 0, 0, 0, 0, time.UTC) latestDate := time.Date(year+1, time.January, 1, 0, 0, 0, 0, time.UTC) diff --git a/tools/cmd/metadataLint.go b/tools/cmd/metadataLint.go new file mode 100644 index 0000000000..a788fdfb53 --- /dev/null +++ b/tools/cmd/metadataLint.go @@ -0,0 +1,85 @@ +/* +Copyright © 2022 Doug Hellmann + +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" + "io/ioutil" + "os" + "path/filepath" + + "github.com/openshift/enhancements/tools/enhancements" + + "github.com/spf13/cobra" +) + +// metadataLintCmd represents the metadataLint command +var metadataLintCmd = &cobra.Command{ + Use: "metadata-lint", + Short: "Check the metadata of the enhancement files given as arguments", + ValidArgs: []string{"enhancement-filename"}, + Run: func(cmd *cobra.Command, args []string) { + errorCount := 0 + for _, filename := range args { + reportError := func(msg string) { + fmt.Fprintf(os.Stderr, "%s: %s\n", filename, msg) + errorCount++ + } + content, err := ioutil.ReadFile(filename) + if err != nil { + reportError(fmt.Sprintf("%s", err)) + continue + } + metadata, err := enhancements.NewMetaData(content) + if err != nil { + reportError(fmt.Sprintf("%s", err)) + continue + } + + // Doc title needs to match filename, minus the ".md" extension + fileBase := filepath.Base(filename) + fileBase = fileBase[0 : len(fileBase)-3] + if fileBase != metadata.Title { + reportError(fmt.Sprintf("the title %s and the file base name %s must match", + metadata.Title, fileBase, + )) + } + + for _, msg := range metadata.Validate() { + reportError(msg) + } + } + + if errorCount > 0 { + fmt.Fprintf(os.Stderr, "%d errors", errorCount) + os.Exit(1) + } + }, +} + +func init() { + rootCmd.AddCommand(metadataLintCmd) + + // Here you will define your flags and configuration settings. + + // Cobra supports Persistent Flags which will work for this command + // and all subcommands, e.g.: + // metadataLintCmd.PersistentFlags().String("foo", "", "A help for foo") + + // Cobra supports local flags which will only run when this command + // is called directly, e.g.: + // metadataLintCmd.Flags().BoolP("toggle", "t", false, "Help message for toggle") +} diff --git a/tools/cmd/pullRequests.go b/tools/cmd/pullRequests.go index b942442de4..b4cdadaa71 100644 --- a/tools/cmd/pullRequests.go +++ b/tools/cmd/pullRequests.go @@ -44,6 +44,7 @@ func newPullRequestsCommand() *cobra.Command { Short: "List pull requests and some characteristics in CSV format", Long: `Produce a CSV list of pull requests suitable for import into a spreadsheet.`, RunE: func(cmd *cobra.Command, args []string) error { + initConfig() earliestDate := time.Now().AddDate(0, 0, daysBack*-1) diff --git a/tools/cmd/report.go b/tools/cmd/report.go index 48c4615a49..cbdd1b1252 100644 --- a/tools/cmd/report.go +++ b/tools/cmd/report.go @@ -23,6 +23,7 @@ func newReportCommand() *cobra.Command { Use: "report", Short: "Generate the weekly activity report", RunE: func(cmd *cobra.Command, args []string) error { + initConfig() earliestDate := time.Now().AddDate(0, 0, daysBack*-1) diff --git a/tools/cmd/reviewers.go b/tools/cmd/reviewers.go index bdd928a950..01b6277218 100644 --- a/tools/cmd/reviewers.go +++ b/tools/cmd/reviewers.go @@ -55,6 +55,8 @@ func newReviewersCommand() *cobra.Command { Short: "List reviewers of PRs in a repo", Long: ``, RunE: func(cmd *cobra.Command, args []string) error { + initConfig() + earliestDate := time.Now().AddDate(0, 0, daysBack*-1) query := &util.PullRequestQuery{ diff --git a/tools/cmd/root.go b/tools/cmd/root.go index 886336d586..88ce470c8b 100644 --- a/tools/cmd/root.go +++ b/tools/cmd/root.go @@ -32,8 +32,6 @@ func init() { defaultConfigFilename := filepath.Join(configDir, "ocp-enhancements", "config.yml") - cobra.OnInitialize(initConfig) - rootCmd.PersistentFlags().BoolVar(&devMode, "dev", false, "dev mode, stop after first page of PRs") rootCmd.PersistentFlags().StringVar(&configFilename, "config", defaultConfigFilename, "config file") rootCmd.PersistentFlags().StringVar(&orgName, "org", "openshift", "github organization") diff --git a/tools/cmd/show-pr.go b/tools/cmd/show-pr.go index caf152f807..257ade6601 100644 --- a/tools/cmd/show-pr.go +++ b/tools/cmd/show-pr.go @@ -33,6 +33,8 @@ func newShowPRCommand() *cobra.Command { return nil }, RunE: func(cmd *cobra.Command, args []string) error { + initConfig() + prID, err := strconv.Atoi(args[0]) if err != nil { return errors.Wrap(err, diff --git a/tools/enhancements/metadata.go b/tools/enhancements/metadata.go index e7b96cb1ab..8507c0d13a 100644 --- a/tools/enhancements/metadata.go +++ b/tools/enhancements/metadata.go @@ -2,6 +2,7 @@ package enhancements import ( "fmt" + "net/url" "github.com/pkg/errors" "gopkg.in/yaml.v2" @@ -43,3 +44,83 @@ func (s *Summarizer) GetMetaData(pr int) (*MetaData, error) { } return NewMetaData(content) } + +// Validate returns a list of issues found with the metadata +func (m *MetaData) Validate() []string { + results := []string{} + + reportError := func(msg string) { + results = append(results, msg) + } + + // Must have one valid tracking link and no TBD + foundLink := false + for _, trackingLink := range m.TrackingLinks { + if trackingLink == "TBD" { + reportError("'TBD' is not a valid value for tracking-link") + continue + } + if trackingLink == "" { + reportError("tracking-link must not be empty") + continue + } + if u, err := url.Parse(trackingLink); err != nil { + reportError(fmt.Sprintf("could not parse tracking-link %q: %s", + trackingLink, err, + )) + continue + } else { + if u.Scheme == "" { + reportError(fmt.Sprintf("could not parse tracking-link %q", + trackingLink, + )) + continue + } + } + foundLink = true + } + if !foundLink { + reportError("tracking-link must contain at least one valid URL") + } + + // No TBD in required people fields + for _, field := range []struct { + Name string + Data []string + }{ + { + Name: "authors", + Data: m.Authors, + }, + { + Name: "reviewers", + Data: m.Reviewers, + }, + { + Name: "approvers", + Data: m.Approvers, + }, + { + Name: "api-approvers", + Data: m.APIApprovers, + }, + } { + valid := 0 + for _, value := range field.Data { + if value == "TBD" { + reportError(fmt.Sprintf("%s must not have TBD as value", field.Name)) + continue + } + if value == "" { + reportError(fmt.Sprintf("%s must not be an empty string", field.Name)) + continue + } + valid++ + } + if valid < 1 { + reportError(fmt.Sprintf("%s must have at least one valid github id", field.Name)) + } + } + + return results +} diff --git a/tools/enhancements/metadata_test.go b/tools/enhancements/metadata_test.go index e4577369c9..42a813f496 100644 --- a/tools/enhancements/metadata_test.go +++ b/tools/enhancements/metadata_test.go @@ -60,3 +60,126 @@ superseded-by: }) } } + +func TestValidateMetaData(t *testing.T) { + for _, tc := range []struct { + Scenario string + Input MetaData + Expected []string + }{ + { + Scenario: "empty", + Input: MetaData{}, + Expected: []string{ + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must have at least one valid github id", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "reviewers-tbd", + Input: MetaData{ + Reviewers: []string{"TBD"}, + }, + Expected: []string{ + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must not have TBD as value", + "reviewers must have at least one valid github id", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "approvers-tbd", + Input: MetaData{ + Approvers: []string{"TBD"}, + }, + Expected: []string{ + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must have at least one valid github id", + "approvers must not have TBD as value", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "reviewers-empty-string", + Input: MetaData{ + Reviewers: []string{""}, + }, + Expected: []string{ + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must not be an empty string", + "reviewers must have at least one valid github id", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "approvers-empty-string", + Input: MetaData{ + Approvers: []string{""}, + }, + Expected: []string{ + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must have at least one valid github id", + "approvers must not be an empty string", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "tracking-link-tbd", + Input: MetaData{ + TrackingLinks: []string{"TBD"}, + }, + Expected: []string{ + "'TBD' is not a valid value for tracking-link", + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must have at least one valid github id", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "tracking-link-parse", + Input: MetaData{ + TrackingLinks: []string{"https//blah/blah"}, + }, + Expected: []string{ + "could not parse tracking-link \"https//blah/blah\"", + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must have at least one valid github id", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + { + Scenario: "tracking-link-emtpy", + Input: MetaData{ + TrackingLinks: []string{""}, + }, + Expected: []string{ + "tracking-link must not be empty", + "tracking-link must contain at least one valid URL", + "authors must have at least one valid github id", + "reviewers must have at least one valid github id", + "approvers must have at least one valid github id", + "api-approvers must have at least one valid github id", + }, + }, + } { + t.Run(tc.Scenario, func(t *testing.T) { + actual := tc.Input.Validate() + assert.Equal(t, tc.Expected, actual) + }) + } +}