Skip to content

Comments

[DOC] SQSERVICES-1500 Document 2nd Factor Password Challenge Team Feature#2329

Merged
battermann merged 5 commits intodevelopfrom
SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com
Apr 28, 2022
Merged

[DOC] SQSERVICES-1500 Document 2nd Factor Password Challenge Team Feature#2329
battermann merged 5 commits intodevelopfrom
SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Apr 28, 2022

@battermann battermann temporarily deployed to cachix April 28, 2022 09:50 Inactive
@battermann battermann changed the title add config options docs to docs.wire.com [DOC] SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com Apr 28, 2022
@battermann battermann force-pushed the SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com branch from 9d28228 to ac9b8c7 Compare April 28, 2022 10:19
@battermann battermann temporarily deployed to cachix April 28, 2022 10:19 Inactive
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

A general comment:

Before, the config-options file in wire-server/docs took the perspective of a wire-server developer, essentially, and giving examples like:

.. code:: yaml
  # brig.yaml
  optSettings:
    setFederationDomain: example.com

So the above snippet has a comment stating brig.yaml. What's brig.yaml exactly? It's the configuration file for the brig binary that, if you compile brig yourself, and run it yourself, with ./dist/brig -c path/to/brig.yaml, will start the brig process (which may fail as it needs databases and other things to be reachable).

This is all fine for a wire-server developer / Wire employee.

But this is not the kind of configuration information anyone reading docs.wire.com under a section "how to install Wire" can make use of. For that audience, it only makes sense to document configuration options at the helm chart value override level, and not at the process' configuration file level.

Now, for this particular example of the federation domain, that is already documented at the helm chart override level, here: https://docs.wire.com/how-to/install/configure-federation.html#configure-helm-charts-federator-and-ingress-and-webapp-subcharts or here: https://github.com/wireapp/wire-server/blob/develop/docs/src/how-to/install/configure-federation.rst#configure-helm-charts-federator-and-ingress-and-webapp-subcharts

As such, the federation domain text in the file you're adding here does not make a lot of sense on its own - it's decoupled from the rest of federation configuration, and it talks at a configuration level no reader can really modify - nobody actually runs brig -c path/to/config.yaml other than (perhaps) Wire backend engineers once per year.

A similar idea holds true for much of the rest of this document - some config options have useful descriptions and comments but may not be modifyable as such in an actual deployment - since e.g. the way to override this reuires scoping them under some additional yaml headers, or since the helm value level configs are named differently to the ones that the haskell processes take.

So in general I think this is very worthwhile work, but it's not sufficient to just move and link docs, the content has to be adpated and partially re-written, and partially deleted (as for the federation things, as those are already covered elsewhere).

Unless you have ideas of structuring docs differently, I would generally write the configuration options in the "helm value override level" perspective, i.e. the way you would structure a myvalues.yaml which one could use with helm (template|install) ./charts/wire-server -f myvalues.yaml.
Separate notes for backend engineer documentation could also be useful, but that should be very clearly marked and put into a part of a documentation that has lots of warnings about this being only useful for developers of the team.

Thanks for working on this!

Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Requesting changes to avoid this being merged too early before addressing my comments.

@battermann
Copy link
Contributor Author

A general comment:

Before, the config-options file in wire-server/docs took the perspective of a wire-server developer, essentially, and giving examples like:

.. code:: yaml
  # brig.yaml
  optSettings:
    setFederationDomain: example.com

So the above snippet has a comment stating brig.yaml. What's brig.yaml exactly? It's the configuration file for the brig binary that, if you compile brig yourself, and run it yourself, with ./dist/brig -c path/to/brig.yaml, will start the brig process (which may fail as it needs databases and other things to be reachable).

This is all fine for a wire-server developer / Wire employee.

But this is not the kind of configuration information anyone reading docs.wire.com under a section "how to install Wire" can make use of. For that audience, it only makes sense to document configuration options at the helm chart value override level, and not at the process' configuration file level.

Now, for this particular example of the federation domain, that is already documented at the helm chart override level, here: https://docs.wire.com/how-to/install/configure-federation.html#configure-helm-charts-federator-and-ingress-and-webapp-subcharts or here: https://github.com/wireapp/wire-server/blob/develop/docs/src/how-to/install/configure-federation.rst#configure-helm-charts-federator-and-ingress-and-webapp-subcharts

As such, the federation domain text in the file you're adding here does not make a lot of sense on its own - it's decoupled from the rest of federation configuration, and it talks at a configuration level no reader can really modify - nobody actually runs brig -c path/to/config.yaml other than (perhaps) Wire backend engineers once per year.

A similar idea holds true for much of the rest of this document - some config options have useful descriptions and comments but may not be modifyable as such in an actual deployment - since e.g. the way to override this reuires scoping them under some additional yaml headers, or since the helm value level configs are named differently to the ones that the haskell processes take.

So in general I think this is very worthwhile work, but it's not sufficient to just move and link docs, the content has to be adpated and partially re-written, and partially deleted (as for the federation things, as those are already covered elsewhere).

Unless you have ideas of structuring docs differently, I would generally write the configuration options in the "helm value override level" perspective, i.e. the way you would structure a myvalues.yaml which one could use with helm (template|install) ./charts/wire-server -f myvalues.yaml. Separate notes for backend engineer documentation could also be useful, but that should be very clearly marked and put into a part of a documentation that has lots of warnings about this being only useful for developers of the team.

Thanks for working on this!

Yes, I completely agree with this.

The reason for the PR is that we need to document how to enable the 2nd factor password challenge team feature, and we thought we could migrate the complete options-config docs at the same time. But as I see now, that this is not a good idea.

Will create a separate PR.

@battermann battermann force-pushed the SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com branch from ac9b8c7 to 45447aa Compare April 28, 2022 11:22
@battermann battermann temporarily deployed to cachix April 28, 2022 11:23 Inactive
@battermann
Copy link
Contributor Author

battermann commented Apr 28, 2022

rendered (outdated):
image

@battermann battermann changed the title [DOC] SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com [DOC] SQSERVICES-1500 Document 2nd Factor Password Challenge Team Feature Apr 28, 2022
@battermann battermann requested a review from jschaul April 28, 2022 11:25
@battermann battermann temporarily deployed to cachix April 28, 2022 11:27 Inactive
@battermann battermann marked this pull request as ready for review April 28, 2022 11:27
@battermann battermann temporarily deployed to cachix April 28, 2022 12:03 Inactive
@battermann battermann temporarily deployed to cachix April 28, 2022 12:07 Inactive
@battermann battermann requested a review from comawill April 28, 2022 14:20
@battermann battermann temporarily deployed to cachix April 28, 2022 14:25 Inactive
@battermann battermann merged commit 7d29c5e into develop Apr 28, 2022
@battermann battermann deleted the SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com branch April 28, 2022 15:14
@battermann battermann restored the SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com branch August 16, 2022 12:44
@battermann battermann deleted the SQSERVICES-1500-doc-move-internal-config-options-docs-to-docs-wire-com branch August 16, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants