Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for symlinks and KEP metadata files #1706

Merged
merged 4 commits into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/kepval/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func run() int {
for _, filename := range os.Args[1:] {
file, err := os.Open(filename)
if err != nil {
fmt.Printf("could not open file: %v", err)
fmt.Printf("could not open file %s: %v\n", filename, err)
return 1
}
defer file.Close()
Expand All @@ -46,6 +46,6 @@ func run() int {
return 1
}

fmt.Printf("No validation errors : %v\n", os.Args[1:])
fmt.Printf("No validation errors: %v\n", os.Args[1:])
return 0
}
29 changes: 26 additions & 3 deletions cmd/kepval/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import (
)

const (
kepsDir = "keps"
kepsDir = "keps"
kepMetadata = "kep.yaml"
)

// This is the actual validation check of all keps in this repo
Expand All @@ -40,7 +41,19 @@ func TestValidation(t *testing.T) {
if info.IsDir() {
return nil
}
if ignore(info.Name()) {

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this result in running this multiple times on any kep.yaml that has a symlink for it's associated README.md? If we are only checking the metadata, I would think instead we would want to:

  1. ignore the symlink
  2. legacy KEP format, we need to check the file
  3. new KEP format, we need to verify a kep.yaml exists, and validate that file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this on purpose to make sure that when you create a symlink, you actually split the old file up correctly for the new format. Processing the kep.yaml files multiple times seems okay to me, the whole process takes a few seconds at most.

Presumably at some point we can remove the old symlinks and drop all of this code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, when there is no error it's fine, when there is an error it's slightly annoying but that's not really a big deal.

We're going to be tweaking all of this code as we evolve the KEP management process, so I am OK with this for now.

return nil
}
if ignore(dir, info.Name()) {
return nil
}
files = append(files, path)
Expand Down Expand Up @@ -69,16 +82,26 @@ func TestValidation(t *testing.T) {
}

// ignore certain files in the keps/ subdirectory
func ignore(name string) bool {
func ignore(dir, name string) bool {
if dir == "../../keps/NNNN-kep-template" {
return true // ignore the template directory because its metadata file does not use a valid sig name
}

if name == kepMetadata {
return false // always check metadata files
}

if !strings.HasSuffix(name, "md") {
return true
}

if name == "0023-documentation-for-images.md" ||
name == "0004-cloud-provider-template.md" ||
name == "YYYYMMDD-kep-template.md" ||
name == "README.md" ||
name == "kep-faq.md" {
return true
}

return false
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ approvers:
- "@bgrant0607"
- "@jbeda"
see-also:
- [/keps/0001-kubernetes-enhancement-proposal-process.md]
- /keps/0001-kubernetes-enhancement-proposal-process.md
replaces: []
1 change: 1 addition & 0 deletions keps/sig-auth/20190711-external-credential-providers.md
7 changes: 7 additions & 0 deletions pkg/kepval/keps/proposals.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func (p *Proposals) AddProposal(proposal *Proposal) {
type Proposal struct {
ID string `json:"id"`
Title string `json:"title" yaml:"title"`
Number string `json:"kep-number" yaml:"kep-number"`
Authors []string `json:"authors" yaml:",flow"`
OwningSIG string `json:"owningSig" yaml:"owning-sig"`
ParticipatingSIGs []string `json:"participatingSigs" yaml:"participating-sigs,flow,omitempty"`
Expand Down Expand Up @@ -83,6 +84,12 @@ func (p *Parser) Parse(in io.Reader) *Proposal {
return proposal
}

// this file is just the KEP metadata
if count == 0 {
metadata = body.Bytes()
proposal.Contents = ""
}

// First do structural checks
test := map[interface{}]interface{}{}
if err := yaml.Unmarshal(metadata, test); err != nil {
Expand Down
10 changes: 7 additions & 3 deletions pkg/kepval/keps/validations/yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,21 +91,25 @@ var listGroups []string
func init() {
resp, err := http.Get("https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml")
if err != nil {
fmt.Fprintf(os.Stderr, "unable to fetch list of sigs: %v", err)
fmt.Fprintf(os.Stderr, "unable to fetch list of sigs: %v\n", err)
os.Exit(1)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
fmt.Fprintf(os.Stderr, "invalid status code when fetching list of sigs: %d\n", resp.StatusCode)
os.Exit(1)
}
re := regexp.MustCompile(`- dir: (.*)$`)

scanner := bufio.NewScanner(resp.Body)
for scanner.Scan() {
match := re.FindStringSubmatch(scanner.Text())
if len(match)>0 {
if len(match) > 0 {
listGroups = append(listGroups, match[1])
}
}
if err := scanner.Err(); err != nil {
fmt.Fprintf(os.Stderr, "unable to scan list of sigs: %v", err)
fmt.Fprintf(os.Stderr, "unable to scan list of sigs: %v\n", err)
os.Exit(1)
}
sort.Strings(listGroups)
Expand Down