From 946942ba6853176b0df962ede8a21bc3831b448e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= Date: Tue, 8 Aug 2023 12:03:55 +0200 Subject: [PATCH] Add package updating/creation tips to dev-docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Weiße --- cli/README.md | 121 ++++++++++++++++++++++++++++++++++++++++ dev-docs/conventions.md | 44 +++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 cli/README.md diff --git a/cli/README.md b/cli/README.md new file mode 100644 index 000000000..e14dd007b --- /dev/null +++ b/cli/README.md @@ -0,0 +1,121 @@ +# Constellation CLI design guide + +This document aims to outline the design principals behind the CLI code and packages. +It should be consulted by developers when making changes to the code of packages in [`cli/internal/`](./internal/). + +## Basic layout + +[`cli/`](./) consists of 2 packages, [`cli/cmd/`](./cmd/), and [`cli/internal/`](./internal/), and a [`main.go`](./main.go), which serves as the entrypoint for the CLI. +No new packages or files should be added to this base directory. + +[`cli/cmd/`](./cmd/) defines the root command of the CLI. Any changes concerning the root command, or additions like persistent flags (flags that can be accessed by all subcommands) should be added here. + +TODO: Discuss: Root command could be added to `cli/internal/cmd/` + +[`cli/internal/`](./internal/) holds the actual logic of the CLI, and will be discussed in detail in the following sections. +The code is kept as internal packages to avoid accidentally exposing non public API logic of the CLI to other packages of Constellation. +Any shared code should instead go into the global [`internal/` package](../internal/). + +## The internal package + +[`cli/internal/`](./internal/) implements logic of the CLI and is split into multiple distinct packages. +Each package should aim to implement a specific functionality. Multi purposes packages should be avoided. + +The following packages are listed and explained in alphabetical order. New packages should be added here when they are created. + +### [`cloudcmd`](./internal/cloudcmd/) + +This package focuses on the interaction with the cloud provider. +It separates the cloud provider specific code from the rest of the CLI, and +provides a common interface for all cloud providers. +Exported functions defined here should be cloud provider agnostic, meaning there should be no functions for specific CSPs. +Rather the `cloudcmd` package should declare functions that take a `cloudprovier.Provider` as argument, +perform CSP specific logic, and return a universally usable result. + +It is used by the [`cmd` package](#cmd) to handle creation of cloud resources and other CSP specific interactions. + +### [`clusterid`](./internal/clusterid) + +Defines the structure of the Constellation cluster ID file. +Logic in this package should be kept minimal. + +### [`cmd`](./internal/cmd) + +Defines the commands of the CLI. +Logic should be kept to input/output parsing whenever possible. +Any more complex code should usually be implemented in one of the other CLI packages. + +The code here should be kept as cloud provider agnostic as possible. +Any CSP specific tasks should be handled by [`cloudcmd`](#cloudcmd). + +All filepaths handled by the CLI code should originate from here. +Common filepaths are defined as constants in [`internal/constants`](../internal/constants/). +To generate workspace correct filepaths for printing, use the functions from [`workspace.go`](./internal/cmd/workspace.go) // TODO: Discuss if we should move this to its own package + +### [`featureset`](./internal/featureset) + +`featureset` defines feature gate constants for the CLI and should not implement any logic. + +### [`helm`](./internal/helm) + +Helm is used to manage Kubernetes deployments of a Constellation cluster. +The package implements functionality to install, update, and manage Helm charts. + +The charts themselves are embedded in a built CLI binary, and values are dynamically updated depending on configuration. +The charts can be found in [`cli/internal/helm/charts/`](./internal/helm/charts/). + +Helm logic should not be implemented outside this package. +All values loading, parsing, installing, uninstalling, and updating of charts should be implemented here. +As such, the number of exported functions should be kept minimal. + +### [`iamid`](./internal/iamid) + +Defines the structure of the IAM service account file. + +TODO: Discuss if this should be renamed. +The cluster ID file was named as such because it contains the cluster ID. +A similar thing is not the case for this file. + +TODO: Discuss if this should be moved. +Having this as a standalone package seems slightly weird, since it contains just CSP specific information generated by Terraform. +Additionally the content of this struct are never written as a file, the output simply printed to the user, or written to their config. +Maybe restructuring this as a return value for the IAM cloud creator would be a better fit. + +### [`kubernetes`](./internal/kubernetes) + +For fetching status information updating resources in Kubernetes not directly managed by Helm, +the CLI needs to interact with the Kubernetes API directly. +This functionality is implemented in the `kubernetes` package. + +The package should be used for: + +* Fetching status information about the cluster +* Creating, deleting, or migrating resources not managed by Helm + +The package should not be used for anything that doesn't just require the Kubernetes API. +For example, Terraform and Helm actions should not be accessed by this package. + +### [`libvirt`](./internal/libvirt) + +Constellation uses libvirt to run clusters on local hardware. +To enable easy deployment across different Linux distributions, the CLI can deploy libvirt inside a Docker container. +The required files and Go code to start the container can be found in [`cli/internal/libvirt/`](./internal/libvirt/). + +The code in this package should be kept minimal, and likely won't need to be changed unless we do a major refactoring of our QEMU/libvirt installation. + +### [`terraform`](./internal/terraform) + +Terraform is used to create cloud resources and IAM resources required for Constellation clusters. +The package implements functionality to create, update, delete, and view said resources. + +The Terraform files are embedded in a built CLI binary, and variables are dynamically update depending on configuration. +The files can be found in [`cli/internal/terraform/terraform/`](./internal/terraform/terraform/). + +Functions of this package should be kept mostly cloud agnostic (There should be no "CreateAzureCluster" function), +as loading the correct values and calling the correct functions for a given CSP is handled by the [`cloudcmd` package](#cloudcmd). + +### [`upgrade`](./internal/upgrade) + +This package implements some logic to upgrade resources for new Constellation releases. + +TODO: Remove this package, since it s functionality should be implemented by other packages. diff --git a/dev-docs/conventions.md b/dev-docs/conventions.md index bd58617f6..26f8ba3fb 100644 --- a/dev-docs/conventions.md +++ b/dev-docs/conventions.md @@ -98,6 +98,50 @@ pkgsite You can now view the documentation on +## Adding to a package + +Adding new functionality to a package is often required whenever new features for Constellation are implemented. + +Before adding to a package, ask yourself: + +* Does this feature implement functionality outside the scope of this package? + * Keep in mind the design goals of the package you are proposing to edit + * If yes, consider adding it to a different package, or [add a new one](#adding-new-packages) +* Does another package already provide this functionality? + * If yes, use that package instead +* Do other parts of Constellation (binaries, tools, etc.) also require this feature? + * If yes, consider adding it an existing, or create a new package, in the global [`internal`](../internal/) package instead. +* Do other parts of Constellation already implement this feature? + * If yes, evaluate if you can reasonably move the functionality from that part of Constellation to the global [`internal`](../internal/) package to avoid code duplication. + +If the answer to all of the questions was "No", extend the package with your chosen functionality. + +Remember to: + +* Update the package description if the package received any major changes +* Add unit tests + +## Adding new packages + +If you are implementing a feature you might find yourself adding code that does not share anything with the existing packages of the binary/tool you are working on. +In that case you might need to add a new package. + +Before adding a new package, ask yourself: + +* Does your new package provide functionality that could reasonably be added to one of the existing packages? + * Keep in mind the design goals of the existing package: Don't add out of scope functionality to an existing package to avoid creating a new one. +* Do other parts of Constellation (binaries, tools, etc.) also require this feature? + * If yes, consider adding the new package to the global [`internal`](../internal/) package. +* Do other parts of Constellation already implement this feature? + * If yes, evaluate if you can reasonably move the functionality from that part of Constellation to the global [`internal`](../internal/) package to avoid code duplication. + +If the answer to all of the questions was "No', add the new package to the binary/tool you are working on. + +Remember to: + +* Add a description to your new package +* Add unit tests + ## CLI reference The command reference within the CLI should follow the following conventions: