Conversation
kawaiwanyelp
left a comment
kawaiwanyelp
left a comment
There was a problem hiding this comment.
The core logic looks good by there are still a number of Pythonisms in here.
| return true | ||
| } | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
You shouldn't use a main function here, since the main function has already been defined here: https://github.com/Yelp/paasta-tools-go/blob/master/cmd/paasta/main.go#L28. Besides, this is not supposed to be the main entry point into the PaaSTA command line tool. Golang isn't like Python, where each file can be a separate script. Instead, you should probably define a validate function, that is called by the "real" main function when paasta validate is run.
| SOAConfigPath string | ||
| } | ||
|
|
||
| func parseFlags(opts *ValidateOptions) error { |
There was a problem hiding this comment.
I think you should take no arguments here and return a *ValidateOptions. It doesn't make sense to initialize the struct outside and pass it in here I think. You can also drop the error return type, since it seems this function has no possibly errors to return (you always return nil).
There was a problem hiding this comment.
i was working on metastatus binary in spare time and have this: https://github.com/Yelp/paasta-tools-go/blob/u/maksym/paasta-metastatus/pkg/cli/options.go
usage example in this file: https://github.com/Yelp/paasta-tools-go/blob/u/maksym/paasta-metastatus/cmd/paasta-metastatus/main.go#L14
maybe some of that code could be ported if useful
| flag.StringVar(&opts.Service, "service", "", "service to validate") | ||
| flag.StringVar(&opts.SOAConfigPath, "yelpsoa-config-root", "", "yelpsoa-configs path") | ||
| flag.Parse() | ||
| return nil |
There was a problem hiding this comment.
validate is meant to be a subcommand of paasta, but you've defined top-level flags here. My suggestion is to use a FlagSet (https://golang.org/pkg/flag/#FlagSet) here instead, which is equivalent to Python's subparser.
| if soa_dir == current_path { | ||
| return soa_dir, nil | ||
| } | ||
| return soa_dir, errors.New("Unknown service") |
There was a problem hiding this comment.
nit: I'd add the service name to the error message. You can format string using fmt.Sprintf.
Optionally. if you wanted to take it a step further, considering this is a common error in PaaSTA, you could define an "UnknownServiceError" as a struct with a Service field, that returns this error message.
| return filepath.Join(soa_dir, service), nil | ||
| } | ||
|
|
||
| current_path, _ := os.Getwd() |
There was a problem hiding this comment.
nit: To follow Go patterns, you should always handle an error, even if that simply means just returning it.
| return false | ||
| } | ||
| if result.Valid() { | ||
| green := color.New(color.FgGreen).SprintFunc() |
There was a problem hiding this comment.
nit: Just like in Python PaaSTA, you may want to eventually turn this into a global function (e.g. PaastaGreen), instead of declaring a local copy here. The same holds for all the other color functions you're using.
| // validateSchema checks if the specified config file has a valid schema. | ||
| // file_path is the path to file to validate. | ||
| // file_type is what schema type should we validate against | ||
| func validateSchema(file_name string, file_type string) bool { |
There was a problem hiding this comment.
In Golang, you should use Camel case instead of Snake case (Pythonic) for everything, including variables (fileName or filename, instead of file_name). You should check your other variables too - there are a lot of cases in this file.
| // validateAllSchemas Finds all recognized config files in service directory,and validates their schema. | ||
| // service_path is the path to location of configuration files. | ||
| func validateAllSchemas(service_path string) bool { | ||
| matches, _ := filepath.Glob(service_path + "/*.yaml") |
There was a problem hiding this comment.
nit: Use filepath.Join, instead of string concatenation.
| matches, _ := filepath.Glob(service_path + "/*.yaml") | ||
| return_code := true | ||
| for _, file_name := range matches { | ||
| file_info, _ := os.Lstat(file_name) |
There was a problem hiding this comment.
Same here, you should at least return your errors.
| if strings.HasPrefix(basename, file_type) == true { | ||
| if validateSchema(file_name, file_type) == false { | ||
| return_code = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of using an if-statement here, you should use a case-statement here, matching over the file type.
Also, you shouldn't need to use == true and == false; just use C-style boolean operators.
There was a problem hiding this comment.
or maybe just
if strings.HasPrefix(basename, file_type) && !validateSchema(file_name, file_type) {
return_code = false
}
?
| @@ -0,0 +1,103 @@ | |||
| { | |||
There was a problem hiding this comment.
are these taken from https://github.com/Yelp/paasta/blob/master/paasta_tools/api/api_docs/swagger.json?
if yes, i would prefer if we copied the whole file and had a test that checks if it's out of sync with content from the url above.
| SOAConfigPath string | ||
| } | ||
|
|
||
| func parseFlags(opts *ValidateOptions) error { |
There was a problem hiding this comment.
i was working on metastatus binary in spare time and have this: https://github.com/Yelp/paasta-tools-go/blob/u/maksym/paasta-metastatus/pkg/cli/options.go
usage example in this file: https://github.com/Yelp/paasta-tools-go/blob/u/maksym/paasta-metastatus/cmd/paasta-metastatus/main.go#L14
maybe some of that code could be ported if useful
3 participants
Sample output looks like: https://fluffy.yelpcorp.com/i/4p6pWN0hC0qW2XgsHflpLTLgL0vtrqFN.html
There are a couple of things missing so far:
1 ) Some checks haven't been implemented yet.
2) Unit Test
3) There are issues to validate Kubernetes and Tron schema. For example, regexp, the standard go package for regular expression that the schema validator uses, doesn't support negative loopahead '?!' for performance issue (It can't be implemented by O(n) time complexity).
4) The green "Yes" and the red "No" need to change to some symbols.