Add Type column in Attribute reference #651
Conversation
| |---------------------|---------------------------------------------------------------------------------------------------------|:--------------:|:---------------------------------------------------------------------------------------------------------------|:--------:| | ||
| | `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 | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I am comfortable with this. So should I close this PR?
There was a problem hiding this comment.
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.
knrt10
force-pushed
the
knrt10/add-type-column-component-attributes
branch
from
be0deaf
to
ab915a7
Compare
Earlier it was difficult to know the type of attribute just by looking into documentation. This patch will help user to know type of attribute. We are using terraform types. This fixes #621 terraform.io/docs/configuration/variables.html#type-constraints for reference. Signed-off-by: knrt10 <kautilya@kinvolk.io>
knrt10
force-pushed
the
knrt10/add-type-column-component-attributes
branch
from
ab915a7
to
173e517
Compare
|
@iaguis updated |
Fixes #621
Signed-off-by: knrt10 kautilya@kinvolk.io