Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Add Type column in Attribute reference #651

Merged
merged 1 commit into from
Jun 26, 2020
Merged

Add Type column in Attribute reference #651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions docs/configuration-reference/backend/local.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ backend "local" {

Default backend is local.

| Argument | Description | Default | Required |
|-----------------------------|--------------------------------------------------------------|:-------:|:--------:|
| `backend.local` | Local backend configuration block. | - | false |
| `backend.local.path` | Location where Lokomotive stores the cluster state. | - | false |
| Argument | Description | Default | Type | Required |
|----------------------|-----------------------------------------------------|:-------:|:------:|:--------:|
| `backend.local` | Local backend configuration block. | - | object | false |
knrt10 marked this conversation as resolved.
Show resolved Hide resolved
| `backend.local.path` | Location where Lokomotive stores the cluster state. | - | string | false |


17 changes: 9 additions & 8 deletions docs/configuration-reference/backend/s3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,15 @@ backend "s3" {

## Attribute reference

| Argument | Description | Default | Required |
|-----------------------------|--------------------------------------------------------------------------------------------------------------|:-------:|:--------:|
| `backend.s3` | AWS S3 backend configuration block. | - | false |
| `backend.s3.bucket` | Name of the S3 bucket where Lokomotive stores cluster state. | - | true |
| `backend.s3.key` | Path in the S3 bucket to store the cluster state. | - | true |
| `backend.s3.region` | AWS Region of the S3 bucket. | - | false |
| `backend.s3.aws_creds_path` | Path to the AWS credentials file. | - | false |
| `backend.s3.dynamodb_table` | Name of the DynamoDB table for locking the cluster state. The table must have a primary key named LockID. | - | false |
| Argument | Description | Default | Type | Required |
|-----------------------------|-----------------------------------------------------------------------------------------------------------|:-------:|:------:|:--------:|
| `backend.s3` | AWS S3 backend configuration block. | - | object | false |
| `backend.s3.bucket` | Name of the S3 bucket where Lokomotive stores cluster state. | - | string | true |
| `backend.s3.key` | Path in the S3 bucket to store the cluster state. | - | string | true |
| `backend.s3.region` | AWS Region of the S3 bucket. | - | string | false |
| `backend.s3.aws_creds_path` | Path to the AWS credentials file. | - | string | false |
| `backend.s3.dynamodb_table` | Name of the DynamoDB table for locking the cluster state. The table must have a primary key named LockID. | - | string | false |


>NOTE: In order for the installer to configure the credentials for S3 backend either pass them as
environment variables or in the config above.
Expand Down
9 changes: 5 additions & 4 deletions docs/configuration-reference/components/aws-ebs-csi-driver.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ Table of all the arguments accepted by the component.

Example:

| Argument | Description | Default | Required |
|--------------------------------|------------------------------------------------------------------------------|:------------:|:--------:|
| `enable_default_storage_class` | Use the storage class provided by the component as the default storage class | true | false |
| Argument | Description | Default | Type | Required |
|--------------------------------|------------------------------------------------------------------------------|-------|-------|--------|
| `enable_default_storage_class` | Use the storage class provided by the component as the default storage class | true | bool | false |


## Applying

Expand All @@ -64,4 +65,4 @@ lokoctl component delete aws-ebs-csi-driver

**WARNING: Before destroying a cluster or deleting the component, EBS volumes
must be cleaned up manually.** Failing to do so would result in EBS volumes
being left behind.
being left behind.
12 changes: 7 additions & 5 deletions docs/configuration-reference/components/cert-manager.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,13 @@ Table of all the arguments accepted by the component.

Example:

| Argument | Description | Default | Required |
|-------------|--------------------------------------------------------------|:------------:|:--------:|
| `email` | Email used for certificates to receive expiry notifications. | - | true |
| `namespace` | Namespace to deploy the cert-manager into. | cert-manager | false |
| `webhooks` | Controls if webhooks should be deployed. | true | false |
| Argument | Description | Default | Type | Required |
|-------------------|----------------------------------------------------------------|:------------:|:------:|:--------:|
| `email` | Email used for certificates to receive expiry notifications. | - | string | true |
| `namespace` | Namespace to deploy the cert-manager into. | cert-manager | string | false |
| `webhooks` | Controls if webhooks should be deployed. | true | bool | false |
| `service_monitor` | Specifies how metrics can be retrieved from a set of services. | false | bool | false |


## Applying

Expand Down
32 changes: 17 additions & 15 deletions docs/configuration-reference/components/cluster-autoscaler.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,21 +70,23 @@ Table of all the arguments accepted by the component.

Example:

| Argument | Description | Default | Required |
|------------------------------|------------------------------------------------------------------------------------------|:---------------|:--------:|
| `cluster_name` | Name of the cluster. | - | true |
| `worker_pool` | Name of the worker pool. | - | true |
| `namespace` | Namespace where the Cluster Autoscaler will be installed. | "kube-system" | false |
| `min_workers` | Minimum number of workers in the worker pool. | 1 | false |
| `max_workers` | Maximum number of workers in the worker pool. | 4 | false |
| `scale_down_unneeded_time` | How long a node should be unneeded before it is eligible for scale down. | "10m" | false |
| `scale_down_delay_after_add` | How long scale down should wait after a scale up. | "10m" | false |
| `scale_down_unready_time` | How long an unready node should be unneeded before it is eligible for scale down. | "20m" | false |
| `provider` | Supported provider, currently Packet. | "packet" | false |
| `packet.project_id` | Packet Project ID where the cluster is running. | - | true |
| `packet.facility` | Packet Facility where the cluster is running. | - | true |
| `packet.worker_type` | Machine type for workers spawned by the Cluster Autoscaler. | "t1.small.x86" | false |
| `packet_worker_channel` | Flatcar Container Linux channel to be used in workers spawned by the Cluster Autoscaler. | "stable" | false |
| Argument | Description | Default | Type | Required |
|------------------------------|------------------------------------------------------------------------------------------|:---------------|:------:|:--------:|
| `cluster_name` | Name of the cluster. | - | string | true |
| `worker_pool` | Name of the worker pool. | - | string | true |
| `namespace` | Namespace where the Cluster Autoscaler will be installed. | "kube-system" | string | false |
| `min_workers` | Minimum number of workers in the worker pool. | 1 | number | false |
| `max_workers` | Maximum number of workers in the worker pool. | 4 | number | false |
| `scale_down_unneeded_time` | How long a node should be unneeded before it is eligible for scale down. | "10m" | string | false |
| `scale_down_delay_after_add` | How long scale down should wait after a scale up. | "10m" | string | false |
| `scale_down_unready_time` | How long an unready node should be unneeded before it is eligible for scale down. | "20m" | string | false |
| `provider` | Supported provider, currently Packet. | "packet" | string | false |
| `service_monitor` | Specifies how metrics can be retrieved from a set of services. | false | bool | false |
| `packet.project_id` | Packet Project ID where the cluster is running. | - | string | true |
| `packet.facility` | Packet Facility where the cluster is running. | - | string | true |
| `packet.worker_type` | Machine type for workers spawned by the Cluster Autoscaler. | "t1.small.x86" | string | false |
| `packet_worker_channel` | Flatcar Container Linux channel to be used in workers spawned by the Cluster Autoscaler. | "stable" | string | false |


## Applying

Expand Down
15 changes: 8 additions & 7 deletions docs/configuration-reference/components/contour.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,14 @@ Table of all the arguments accepted by the component.

Example:

| Argument | Description | Default | Required |
|---------------------|---------------------------------------------------------------------------------------------------------|:--------------:|:--------:|
| `enable_monitoring` | Create Prometheus Operator configs to scrape Contour and Envoy metrics. Also deploys Grafana Dashboard. | false | false |
| `ingress_hosts` | [ExternalDNS component](external-dns.md) creates DNS entries from the values provided. | "" | false |
| `node_affinity` | Node affinity for deploying the operator pod and envoy daemonset. | - | false |
| `service_type` | The type of Kubernetes service used to expose Envoy. | "LoadBalancer" | false |
| `toleration` | Tolerations that the operator and envoy pods will tolerate. | - | false |
| Argument | Description | Default | Type | Required |
|---------------------|---------------------------------------------------------------------------------------------------------|:--------------:|:---------------------------------------------------------------------------------------------------------------|:--------:|
| `enable_monitoring` | Create Prometheus Operator configs to scrape Contour and Envoy metrics. Also deploys Grafana Dashboard. | false | bool | false |
| `ingress_hosts` | [ExternalDNS component](external-dns.md) creates DNS entries from the values provided. | "" | list(string) | false |
| `node_affinity` | Node affinity for deploying the operator pod and envoy daemonset. | - | list(object({key = string, operator = string, values = list(string)})) | false |
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, if we have a list or map of blocks, those should get separate table with parameters. It will be much more readable then.

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to sidetrack the discussion too much, but I've been wanting to re-evaluate using tables for a while now, so if we figure tables are causing trouble with this PR, maybe it's time to question this decision.

Rationale:

I find tables too limiting for a config reference as there isn't a lot of space in a table cell for really explaining what some knob is/does and also there is no convenient way to add note blocks or code snippets to help explain some point.

I'd rather have a more flexible format which allows us to write as much text as we want to explain a knob. I remind us that a config reference should be exhaustive rather than concise, i.e. "if it's not in the reference, it doesn't exist". I don't have a specific format in mind but am happy to propose one if folks are open to that.

Copy link
Member

Choose a reason for hiding this comment

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

A concrete example:

The description for cluster_name is Name of the cluster.. IMO this description is nearly useless as it doesn't tell the user what would happen if they put one value or the other in this field. I'd rather include a longer text which explains how this value is used by lokoctl, however I'd need some space to do that if I don't want to have a really tall table cell with lots of line breaks.

Copy link
Member

Choose a reason for hiding this comment

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

@johananl what are you suggesting we should do then? Should we keep it as it is? Should we not add type for lists and maps for now?

Copy link
Member

@johananl johananl Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have nothing against adding a type column, I wrote the above since it sounded to me like we were discussing the overall structure of the document and wanted to see if it's an opportunity to rethink our usage of tables.

I'm leaning towards a paragraph-based format with a ToC at the top, a bit like this maybe. This format allows a lot of flexibility since you can provide full-text paragraphs for each knob, there is room for notes and sample snippets etc. But this is likely out of scope for this PR. Again, I wanted to mainly give this as something to think about if we are re-evaluating the overall structure here.

Copy link
Contributor

Choose a reason for hiding this comment

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

To move forward this how about leaving it like it is now (even if it's not very readable) and creating an issue to switch to a paragraph-based format?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am comfortable with this. So should I close this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd merge this PR if it looks OK (it adds valuable information) and then we can work on moving to a paragraph-based format. I'll review this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool

| `service_type` | The type of Kubernetes service used to expose Envoy. | "LoadBalancer" | string | false |
| `toleration` | Tolerations that the operator and envoy pods will tolerate. | - | list(object({key = string, effect = string, operator = string, value = string, toleration_seconds = string })) | false |


## Applying

Expand Down
Loading