-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize component config introductions/options #4189
base: main
Are you sure you want to change the base?
Conversation
👋 🤖 🤔 Hello! Did you make your changes in all the right places? These files were changed only in docs/. You might want to duplicate these changes in versioned_docs/version-8.6/.
You may have done this intentionally, but we wanted to point it out in case you didn't. You can read more about the versioning within our docs in our documentation guidelines. |
CC @camunda/distribution for review here 👍 |
@@ -9,14 +9,21 @@ description: "Read details on the configuration variables of Console Self-Manage | |||
Console Self-Managed is available only to [Enterprise customers](/reference/licenses.md#console). | |||
::: | |||
|
|||
Console Self-Managed can be configured using environment variables and configuration parameters. | |||
Console can be configured using environment variables, configuration parameters, or a combination of both. When configuring your Console setup, keep in mind the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a PM question, but do we want to emphasize "Console Self-Managed" as the component name? Or are they similar enough that we can drop "Self-Managed" with the right context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a good idea to specify self-managed as well, since it is a different development stream. It is quite different from SaaS.
cc @theburi
@@ -71,7 +80,7 @@ console: | |||
value: online | |||
``` | |||
|
|||
## Using a different OpenID Connect (OIDC) authentication provider than Keycloak | |||
## Configure an OpenID Connect (OIDC) provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is still a good idea to mention keycloak somewhere in the title, just so it is clear that the OIDC provider and keycloak are mutually exclusive.
Or maybe mention it in a paragraph under the title.
What do you think @conceptualshark ?
## Environment variables | ||
|
||
:::note | ||
Underscores in environment variables correspond to configuration file key levels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is true for all configuration options in identity, but I don't think it holds true for other components. I think it would be better to use Generally,...
at the start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@conceptualshark
I have a stronger opinion about this now. It is not correct to say "Underscores in environment variables correspond to configuration file key levels."
This does not hold true in many scenarios
Removing |
Removed the less-certain pieces of this update (if underscores always represent key levels) and included feedback re: Keycloak specificity, specifying Console Self Managed. Also removed a reference to Console being enterprise-specific and will follow up on other instances of that in a separate issue. |
The preview environment relating to the commit b148542 has successfully been deployed. You can access it at https://preview.docs.camunda.cloud/pr-4189/index.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this! 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 No concerns.
I did notice some repeated content that could be added to a partial/reusable component, but would consider that a good item to follow up on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I would like to see a review from someone on the SM side, I don't see anything that makes me pause with the changes in this PR. With my approval, we could merge.
Up to you at this point @conceptualshark if you want to prompt for more reviewers.
@theburi @hamza-m-masood Is it possible to get another final sign-off here? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have raised my concerns in this comment:
#4189 (comment)
Description
An attempt to start the component configuration pages on the path of some sort of standardization. This was scoped down to only tackle the introduction and config options for each page, and is not meant to tackle bigger areas of restructuring, rewriting, etc.
When should this change go live?
hold
label or convert to draft PR)PR Checklist
/versioned_docs
directory./docs
directory (aka/next/
).