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 command validate to check for parse errors #62

Merged
merged 17 commits into from
May 23, 2019
Merged

Add command validate to check for parse errors #62

merged 17 commits into from
May 23, 2019

Conversation

apoorvam
Copy link
Collaborator

Adds a command validate to check for parse errors #31 #55

Running command dunner validate will check for any parse errors and warnings. If there are any errors, it fails the cmd. If not it finishes by listing any warnings if present.

Example:

$ dunner validate
ERRO[2019-03-22 17:15:56] dunner: [stats] Image repository name cannot be empty

Please let me know if there are any other validations for configs to be included.

Copy link
Collaborator

@ayushjn20 ayushjn20 left a comment

Choose a reason for hiding this comment

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

Superb work!
You could have added some more validations, which in my opinion, are important ones like,

  • Directories listed for mounting on containers are present on host or not.
  • Task is present or not when called as @taskName in a step of some other task.
  • Sufficient number of arguments passed when one task is used as step for another task.

@apoorvam
Copy link
Collaborator Author

@ayushjn20 You're right, We need to look for all kind of other validations. I'll try add the ones you suggested.

@agentmilindu
Copy link
Contributor

Is there something similar to Joi for Golang?

@agentmilindu
Copy link
Contributor

Can we use https://github.com/go-playground/validator?

See https://github.com/go-playground/validator/blob/v9/_examples/simple/main.go

@apoorvam
Copy link
Collaborator Author

apoorvam commented Mar 29, 2019

I gave a shot at the go-playground/validator package. This is how it would look:

type Task struct {
	Name    string   `yaml:"name"`
	Image   string   `yaml:"image" validate:"required"`
	SubDir  string   `yaml:"dir"`
	Command []string `yaml:"command" validate:"required,gt=0,dive,required"`
	Envs    []string `yaml:"envs"`
	Mounts  []string `yaml:"mounts"`
	Args    []string `yaml:"args"`
}

type Configs struct {
	Tasks map[string][]Task `validate:"required,gt=0,dive,keys,required,endkeys,required,gt=0,dive,required"`
}

func (configs *Configs) Validate() ([]error) {
	var errs []error
	err := validator.New().Struct(configs)
	if (err != nil) {
		if _, ok := err.(*validator.InvalidValidationError); ok {
			errs = append(errs, err)
		} else {
			for _, e := range err.(validator.ValidationErrors) {
				errs = append(errs, fmt.Errorf("dunner: Validation for field '%s' failed on %s tag", e.Field(), e.Tag()))
			}
		}
	}
	return errs
}

  • I think it can get complicated and requires to understand the DSL defined by library.
  • We cannot differentiate warnings and errors.
  • Error messages are not easy to be made user-friendly. The above code would give an error message like: dunner: Validation for field 'Command' failed on gt tag
    To give more appropriate error message, we'll have to use translations like this: https://github.com/Aptomi/k8s-app-engine/blob/master/pkg/lang/validation.go#L98

Advantage is that messages can be uniform and easy to add more validations. I'll dig up more on if its worth it or probably write a custom validator.
@agentmilindu / all, Let me know if you have any thoughts on this.

@agentmilindu
Copy link
Contributor

@apoorvam

  1. https://www.slideshare.net/surajssd009005/jsonschema-with-golang
  2. https://stackoverflow.com/a/53230568/1190184

@apoorvam apoorvam changed the base branch from master to develop May 15, 2019 19:14
@apoorvam
Copy link
Collaborator Author

Update:

  • dunner validate checks for any parse errors and validation errors. If there are any, it fails with non-zero exit code.
  • Same validation is performed before dunner do <task> also.
  • If any required fields are missing/empty, it gives error as below. It prefixes each error with task name so its easy to track error.
task 'stats': image is a required field
  • In case of mount dir, it fails if its not in right format. Or if directory is not present in the host machine.
$ dunner validate
Validation failed for following errors:
task 'test': mount directory '/tmp/sample:/tmp' is invalid. Check format is '<valid_src_dir>:<valid_dest_dir>:<mode>' and has right permission level
  • Added framework for add custom validations along with required context info.

Task is present or not when called as @taskName in a step of some other task.
Sufficient number of arguments passed when one task is used as step for another task.

Above two validations should be added after this change to use follow is implemented. Else it is just redundant. Will make it a part of new issue.

@agentmilindu @ayushjn20 Please review.

@apoorvam apoorvam added the gsoc-2019 Implemented as part of GSoC 2019 label May 19, 2019
@agentmilindu agentmilindu changed the base branch from develop to master May 19, 2019 21:06
@agentmilindu agentmilindu changed the base branch from master to develop May 19, 2019 21:07
@agentmilindu agentmilindu changed the base branch from develop to master May 20, 2019 07:42
@agentmilindu agentmilindu changed the base branch from master to develop May 20, 2019 07:42
@apoorvam apoorvam requested a review from ayushjn20 May 20, 2019 14:59
@agentmilindu agentmilindu merged commit 8becd1b into leopardslab:develop May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc-2019 Implemented as part of GSoC 2019
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants