Code Structure and Coding Practices

Establish and document codebase conventions early and often

As we build out the code base we should establish clear and well-documented conventions around naming, directory structure, common interfaces, etc. If adding a new convention will help your current work, then float it with other team members and then lock it in if the team agrees.

Presenters

Both CCNG and the CRD spike use presenters to translate our internal representations of objects into user-facing forms. We should continue this pattern. Presenters for single-resource endpoints (e.g. show, create) can be reused in multi-resource endpoints (e.g. index) via composition, though there are some cases where certain fields are omitted in the list context, so we’ll have to handle that via presenter parameters or something similar.

Use Message types for parsing each endpoint’s payload

For example, you might have a CreateAppPayload and an UpdateAppPayload, each of which has only the fields for that specific endpoint.

In CCNG these messages also validate their data: an example. We could either continue this pattern or separate the data object and validation concerns. These validations should only rely on information contained in the message itself (e.g. no k8s API lookups)

Prefer interfaces to struct types for dependencies

Interfaces make it easier to substitute mocks in during unit testing, and also make it simpler to change behavior down the line by swapping in a different concrete struct type. Wherever possible we should use interfaces for dependencies. If we test drive everything with unit tests then this will come naturally.

Testing Standards

We should strive for a “testing pyramid”, with more unit tests and fewer higher-cost integration and end-to-end (e2e) tests. One strategy to accomplish this is to only write integration tests for “common path” scenarios (also known as “happy path”, though this obscures that some common cases are error cases, such as invalid inputs).

Unit tests generally only test a single component (“unit”) and mock out dependencies. This makes it possible to test edge cases that are difficult to reproduce in more realistic setups. The tests also act as clear documentation for the behavior of “units”. Because go is a statically typed language, many of the unit tests that are necessary in dynamic languages like Ruby can be skipped. One rule of thumb for whether to write a test case is “will this test a branch or loop?” If the answer is no, then you can likely skip writing that test.

Integration tests are well suited for scenarios that cross more than one component of the system and can be run in an isolated context (e.g. request specs in Rails, http handler tests with mocked API/DB clients in go). In the context of the SHIM, tests backed by envtest can be considered “integration”, since the k8s API is faked out and running locally.

E2E tests integrate with external components (e.g. the k8s API) which means they provide a higher level of validation. They are also the most expensive tests to maintain and most likely to be flakey in practice. These are best for testing entire “common path” workflows that cross multiple API endpoints. We should only write these for scenarios that can’t be handled by unit tests or integration tests. These tests will likely involve deploying an image with the SHIM to a real k8s cluster, though it may also make sense to run the SHIM locally and integrate against a real remote cluster instead. It’s also likely that other components will need to be installed on the cluster, such as our custom CRD controllers.

Consider sidecar containers for specialized tasks

For example, we may be able to shunt aspects of authentication into a separate code base / container image and remove that concern from the api codebase. It may prove simpler to handle this all in the same code base, but consider the option.

Use wrapped errors to provide context

See https://peter.bourgon.org/blog/2019/09/11/programming-with-errors.html. When your code receives an error from a dependency, you should add additional context to the error via fmt.Errorf(“<context here>: %w”, err)

Use custom error types to improve code clarity

If your code needs to respond differently depending on the type of the error that occurs, then consider creating a custom error type to handle the scenario. The errors.Is() and errors.As() functions can tell you whether an error is of a specific type or wraps an error of that type.

Here’s an example:

err := doTheThing()
if err != nil {
  if errors.Is(mypackage.BoomError) {
    fmt.Printf("Boom! %v", err.Error())
  } else {
    fmt.Printf("No boom today. Boom tomorrow. There's always a boom tomorrow.")
  }
}

Log everything that may be useful in debugging

...but use log levels so we can turn off the less essential logging in production contexts. The built-in golang logger doesn’t support log levels, but there are several packages that do, including logrus