Skip to content

Commit

Permalink
docs: Update to reflect the current state of the project (#347)
Browse files Browse the repository at this point in the history 
Updates the developer docs with current information about the state
of the project.

Some of the information removed from the STYLEGUIDE.md will be moved
to cloud-sdk-go where it is relevant.
  • Loading branch information
@karencfv
karencfv committed Jul 20, 2020
1 parent 500e04a commit 42ce7af
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 122 deletions.
88 changes: 33 additions & 55 deletions developer_docs/NEW_COMMAND.md
View file
Expand Up @@ -10,67 +10,53 @@ into the `app` package for business logic and returns results.

[`cmd/root.go`](../cmd/root.go) contains the root command (`ecctl`) and global flags.
[`cmd/commands.go`](../cmd/commands.go) attaches the top level commands (`deployment`, `platform`, etc) to the root command.
The subdirectories define the subcommand structure (e.g. `platform` -> `proxy`). The lowest level `command.go`
(e.g. [`cmd/platform/proxy/command.go`](../cmd/platform/proxy/command.go)) contains the definition of the actual command
(e.g. `platform proxy list`).
The subdirectories define the subcommand structure (e.g. `deployment` -> `list`). The lowest level
(e.g. [`cmd/deployment/list.go`](../cmd/deployment/list.go)) contains the definition of the actual command
(e.g. `deployment list`).

### Example: adding a new command to list proxies
### Example: adding a new command to list deployments

Create a new subdirectory `cmd/platform/proxy`, and add a `command.go` file to it. (Some of these files/directories
might already exist, e.g. if you are adding a new operation for an existing entity.) You will define your new command here.
Create a new subdirectory `cmd/deployment`, and add a `list.go` file to it. Some of these files/directories
might already exist (i.e. if you are adding a new operation for an existing entity). You will define your new command here.

First, you need to define the `proxy` command:
Define the `list` subcommand in `cmd/deployment/list.go`:

```go
var Command = &cobra.Command{
Use: "proxy",
// ...
}
```

Next, define the `list` subcommand:

```go
var listProxiesCmd = &cobra.Command{
var listCmd = &cobra.Command{
Use: "list",
// ...
}
```

There are a few things to note about the `list` command:

- The `list` command will contain a call to the `proxy.List()` method from the `pkg/platform/proxy` package which you have
not defined yet. You will define this method later. You will also need to define the parameters struct used to pass
arguments to this method - e.g. `proxy.ListParams`.
- Ideally, you would implement a custom text formatter to present the proxy list in a user-friendly way. We currently
- The `list` command will contain a call to the `deploymentapi.List()` function from the [`/pkg/api/deploymentapi`](https://github.com/elastic/cloud-sdk-go/tree/master/pkg/api/deploymentapi)
package found in the cloud-sdk-go. You will define this function later. You will also need to define the parameters
struct used to pass arguments to this function - e.g. `deploymentapi.ListParams`.
- Ideally, you would implement a custom text formatter to present the deployment list in a user-friendly way. We currently
support two types of output: json and text. Absent a custom formatter, the output defaults to *json*. If you choose to add
a formatter to support *text* output, you'll need to create a template such as
[`pkg/formatter/templates/text/proxy/list.gotmpl`](../pkg/formatter/templates/text/proxy/list.gotmpl). To ensure this template
gets used, invoke a `ecctl.Get().Formatter.Format()` from the [`listProxiesCmd`](https://github.com/elastic/ecctl/blob/a90daa0c4411905c8d5c3fa06f5b6250395c4730/cmd/platform/proxy/command.go#L60).
[`pkg/formatter/templates/text/deployment/list.gotmpl`](../pkg/formatter/templates/text/deployment/list.gotmpl). To ensure this template
gets used, invoke a `ecctl.Get().Formatter.Format()` from the [`listCmd`](https://github.com/elastic/ecctl/blob/master/cmd/deployment/list.go#L39).

Next, the `init()` function attaches the `list` subcommand to the `proxy` command, and adds any custom
Next, the `init()` function attaches the `list` subcommand to the `deployment` command, and adds any custom
command line parameters.

```go
func init() {
Command.AddCommand(listProxiesCmd)
Command.AddCommand(listCmd)
// ...
}

```

Finally, you need to attach the `proxy` subcommand to the `platform` command, by adding a reference to it
in [`cmd/platform/platform.go`](../cmd/platform/platform.go).
Finally, you need to attach the `deployment` subcommand to the `ecctl` command, by adding a reference to it
in [`cmd/commands.go`](../cmd/commands.go).

## The `pkg` package

The [`pkg`](../pkg) package is used by the `cmd` package to create commands and is library code that's ok to use by external applications.

### [`deployment`](../pkg/deployment)

Business logic behind `deployment` commands. Typically, methods in this package are called from the `cmd` package,
they accept parameters and call into the ES Cloud API, process and return results to the commands.

### [`ecctl`](../pkg/ecctl)

Contains the business logic and configuration for the ecctl app.
Expand All @@ -79,39 +65,32 @@ Contains the business logic and configuration for the ecctl app.

Functions and templates to format command output from json into user friendly text.

### [`platform`](../pkg/platform)

Business logic behind `platform` commands. Typically, methods in this package are called from the `cmd` package,
they accept parameters and call into the ES Cloud API, process and return results to the commands.

### [`util`](../pkg/util)

Common resources, such as utility functions, constants, parameters, that are shared among different packages.

### Example: adding a new command to list proxies (application logic)
### Example: adding a new command to list deployments (application logic)

Next, you need to add the business logic to your `platform proxy list` command. That's the `List()`
method mentioned earlier. It should go into the `pkg/platform/proxy/proxy.go` file (that you'll need create if it does
not already exist):
Next, you need to add the business logic to your `ecctl deployment list` command. That's the `List()`
function mentioned earlier. These APIs can be found in [cloud-sdk-go](https://github.com/elastic/cloud-sdk-go/tree/master/pkg/api).
The API for our list command should go in the [`cloud-sdk-go/pkg/api/deploymentapi/list.go`](https://github.com/elastic/cloud-sdk-go/tree/master/pkg/api/deploymentapi/list.go) file
which you'll need create if it does not already exist:

```go
func List(params ListParams) (*models.ProxyOverview, error) {
func List(params ListParams) (*models.DeploymentsListResponse, error) {
// ...
}
```

Things to note about the `List` function:

- This is where the main business logic happens: it should include a call to the cloud API to retrieve all the
proxies and return them to the calling function.
- Make sure to properly catch and handle all possible errors. Consider using the `multierror`
library if that makes sense. In this simple case, where we only have one API call, and not much else in terms of processing,
it's probably fine to use regular `errors`.
- For a general set of guidelines on how to write good Go code, see our [Style Guide](https://github.com/elastic/ecctl/blob/master/developer_docs/STYLEGUIDE.md).
- This is where the main business logic happens: it should include a call to the Elastic Cloud API to retrieve all the
deployments and return them to the calling function.
- Make sure to properly catch and handle all possible validation errors. Use the [`multierror`](https://github.com/elastic/cloud-sdk-go/blob/master/pkg/api/deploymentapi/get.go#L46-L57)
package even if you're returning a single error. This provides consistency and good UX since the errors will be be properly prefixed.
- For a general set of guidelines on the project's code style, see our [Style Guide](https://github.com/elastic/ecctl/blob/master/developer_docs/STYLEGUIDE.md).

You'll also need to define the `ListParams` struct, as we normally use structs to pass several arguments to functions. If you only have one or two
functions and corresponding parameter structs, it's ok to define the parameters structs in the same file. If it starts growing beyond that,
a good practice is to define parameter structs in their own file - in this case it would be `pkg/platform/proxy/proxy_params.go`.
You'll also need to define the `ListParams` struct, as we normally use structs to pass several arguments to API functions.

```go
type ListParams struct {
Expand All @@ -128,14 +107,13 @@ func (params ListParams) Validate() error {
```

Additionally, please create unit tests for your changes. Both the `List()` function and the `Validate()`
method for `ListParams` need to be tested. These tests should go to `pkg/platform/proxy/proxy_test.go`
(and `pkg/platform/proxy/proxy_params_test.go`, if you separated the params in their own file).
method for `ListParams` need to be tested. These tests should go to `cloud-sdk-go/pkg/api/deploymentapi/list_test.go`
There are many examples of this in the code base, so feel free to browse and use them as inspiration.

That concludes all the steps necessary for creating a new command. You can easily manually test your new command by
making use of our [helper scripts](../CONTRIBUTING.md#helpers):
importing your local cloud-sdk-go changes running `make fake-sdk`, and making use of our [helper scripts](../CONTRIBUTING.md#helpers):

`dev-cli --config ~/path/to/your/ecctl/config.yaml platform proxy list`
`dev-cli --config config deployment list`

If your command behaves as expected, all that's left is to make sure you followed all the
[code contribution guidelines](../CONTRIBUTING.md#code-contribution-guidelines) before submitting your PR.
Expand Down
79 changes: 12 additions & 67 deletions developer_docs/STYLEGUIDE.md
View file
Expand Up @@ -6,7 +6,7 @@
- [API Errors](#api-errors)
- [Multiple errors](#multiple-errors)
- [Packages](#packages)
- [Structure](#structure)
- [Command Structure](#command-structure)
- [Util packages](#util-packages)
- [Testing](#testing)
- [Unit tests](#unit-tests)
Expand Down Expand Up @@ -37,19 +37,15 @@ All errors must be handled and returned, `_` variables must not be used to disca

#### Exceptions

Parsing command line arguments/flags is done like this
Parsing command line arguments/flags is done like this:

```go
threshold, _ := cmd.Flags().GetUint32(thresholdArg)
```

The above line would return an error only if the flag is not defined, or the datatype does not match the flag declaration data type.
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.

### API Errors

API errors should always be encapsulated with `apierr.Unwrap()`, this function tries to break down and inspect the encapsulated and multi-layer wraps that the API errors contain.

### Multiple errors

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.
Expand Down Expand Up @@ -89,15 +85,13 @@ func (params StopParams) Validate() error {

## Packages

### Structure

A package per context, a context applies to any of the high level containers like `platform`, `deployment`, etc. When a context becomes too large to be contained in a package we can start breaking it down into sub-packages.
### Command Structure

An example would be [pkg/deployment/elasticsearch/plan](../pkg/deployment/elasticsearch/plan).
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>`.

### Util packages

When a function can be made generic it should go in one of the utils packages (e.g. [pkg/util](../pkg/util), [pkg/util](../pkg/util)) to remove complexity and give the ability to be reused.
When a function can be made generic it should go in one of the utils packages (e.g. [cmd/util](../cmd/util), [pkg/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](https://github.com/elastic/cloud-sdk-go) or a standalone repository if the functionality is big enough.

Expand All @@ -107,40 +101,11 @@ If the function is not specific to `ecctl`, it should be part of [cloud-sdk-go](

All files containing functions or methods must have a corresponding unit test file, and we aim to have 100% coverage.

#### API Mocks
#### Testing commands

When unit testing functions which will call the external API, please use the provided `api.NewMock` in conjunction with `mock.Response`.
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..

yes! :smile:

``` go
import (
"net/http"

"github.com/elastic/cloud-sdk-go/pkg/api"
"github.com/elastic/cloud-sdk-go/pkg/api/mock"
)

//Test case
{
name: "succeeds",
args: args{params{
API: api.NewMock(mock.Response{
Response: http.Response{
Body: mock.NewStringBody(`{}`),
StatusCode: 200,
},
}),
}},
},
// More tests ...
```

### Testing commands

When writing unit tests for commands, we only look to assert that the command is constructing the correct API call. API responses are mocked and tested only in the `pkg/` directory.

See [TestRunShowKibanaClusterCmd()](./cmd/kibana/show_test.go) as a good example to base your tests on.
See [Test_listCmd()](../cmd/deployment/template/list_test.go) as a good example to base your tests on.

## General Style

Expand Down Expand Up @@ -179,33 +144,13 @@ Names should be descriptive and we should avoid redundancy.
yes! :smile:

```go
kibana.Create()
```

preferably not :confused:

```go
kibana.CreateKibanaDeployment()
```

When using method chaining make sure to put each method in it's own line to improve readability.

yes! :smile:

```go
res, err := a.API.V1API.ClustersApm.GetApmClusterPlanActivity(
clusters_apm.NewGetApmClusterPlanActivityParams().
WithClusterID(params.id).
WithShowPlanDefaults(params.defaults),
)
ecctl.Get()
```

preferably not :confused:

```go
res, err := a.API.V1API.ClustersApm.GetApmClusterPlanActivity(
clusters_apm.NewGetApmClusterPlanActivityParams().WithClusterID(params.id).WithShowPlanDefaults(params.defaults),
)
ecctl.GetEcctlInstance()
```

When possible we try to avoid `else` and nested `if`s. This makes our code more readable and removes complexity.
Expand Down Expand Up @@ -241,7 +186,7 @@ if params.Hide {

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 elasticsearch list](cmd/deployment/elasticsearch/list.go) command.
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](../cmd/deployment/create.go) 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.

Expand Down

0 comments on commit 42ce7af

Please sign in to comment.