Our codebase is structured very similarly to what DDD (Domain Driven Design) dictates, and even though there’s no strict naming or patterns being enforced we try to follow it as a good practice.
All errors should be informative and helpful. Something like "a cluster ID is required for this command" is more useful than "no ID". This is a helpful blog post on the subject: How to Write Good Error Messages.
Error strings should not be capitalised or end with punctuation, since they are usually printed following other context.
In regards to packages, we prefer to use the "errors" standard library package over "github.com/pkg/errors"
All errors must be handled and returned, _ variables must not be used to discard them.
Parsing command line arguments/flags is done like this:
threshold, _ := cmd.Flags().GetUint32(thresholdArg)The above line would return an error only if the flag is not defined, or the data type does not match the flag declaration data type.
Adding many if err checks will make the code a little bit noisy so we can ignore the errors in these cases.
When multiple errors can be returned, it is preferable to use the mutlierror.Prefixed type to return all the possible errors with a prefixed string to include some context.
yes! 😄
func (params StopParams) Validate() error {
var merr = multierror.NewPrefixed("stop operation")
if params.ID == "" {
merr = merr.Append(errors.New("ID cannot be empty"))
}
if params.API == nil {
merr = merr.Append(errors.New("api reference is required"))
}
return merr.ErrorOrNil()
}preferably not 😕
func (params StopParams) Validate() error {
if params.ID == "" {
return errors.New("ID cannot be empty")
}
if params.API == nil {
return errors.New("api reference is required")
}
return nil
}Commands are defined following the structure of the Elastic Cloud API. If a request to the API is GET /api/v1/deployments/{deployment_id}, the corresponding command will be ecctl deployment show <deployment_id>.
When a function can be made generic it should go in one of the utils packages (e.g. cmd/util, pkg/util) to remove complexity and give the ability to be reused.
If the function is not specific to ecctl, it should be part of cloud-sdk-go or a standalone repository if the functionality is big enough.
All files containing functions or methods must have a corresponding unit test file, and we aim to have 100% coverage.
When writing unit tests for commands, we use the testutils.RunCmdAssertion() function which tests a *cobra.Command and uses the testing.T struct to return any unmatched assertions..
See Test_listCmd() as a good example to base your tests on.
Before committing to your feature branch, run make lint and make format to ensure the code has the proper styling format. We run linters on Jenkins, so this also prevents your builds from failing.
Unless a pointer is necessary, always use the value type before switching to the pointer type. Of course, if your structure contains a mutex, or needs to be synced between different goroutines, it needs to be a pointer, otherwise there’s no reason why it should be.
To remove complexity, when it's not necessary to set length or capacity, it's preferable to declare a nil slice as it has no underlying array. We should avoid using the make function, as this function allocates a zeroed array that returns a slice of said array.
yes! 😄
var slice []stringpreferably not 😕
slice := make([]string, 0)A Config or Params structure is used to encapsulate all the parameters in a structure that has a .Validate() error signature, so it can be validated inside the receiver of that structure.
Unless the Params struct needs to satisfy the pool.Validator interface for concurrency on that receiver it should always remain a value type and not a pointer type.
Names should be descriptive and we should avoid redundancy.
yes! 😄
ecctl.Get()preferably not 😕
ecctl.GetEcctlInstance()When possible we try to avoid else and nested ifs. This makes our code more readable and removes complexity.
Specifically, we prefer having multiple return statements over having nested code.
yes! 😄
if params.Hide {
params.Do()
return data, nil
}
if isHidden {
return nil, fmt.Errorf("example error", params.Name)
}
return data, nilpreferably not 😕
if params.Hide {
params.Do()
} else {
if isHidden {
return nil, fmt.Errorf("example error", params.Name)
}
}We use make docs to automatically generate documentation for our commands which live in the cmd folder.
It is important when writing the descriptions to our commands or flags, that we use simple language and are as clear as possible to provide good UX. If you need to explain more about the command or give examples, please do so using the Example field, a good example is the deployment list command.
The package wide description and documentation is provided in a godoc doc.go file. Aside form packages with a very small context, all packages should have this file.
For further information on good practices with Go in general, check out this document.