Skip to content

Add tt cluster reference #4046

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

Merged
merged 7 commits into from
Feb 20, 2024
Merged

Add tt cluster reference #4046

merged 7 commits into from
Feb 20, 2024

Conversation

p7nov
Copy link
Contributor

@p7nov p7nov commented Feb 16, 2024

Resolves #3725, #3929

Added a new page for the tt cluster command reference: tt cluster

@p7nov p7nov force-pushed the gh-3725-tt-cluster branch from 90a67fd to b95236d Compare February 16, 2024 05:34
@p7nov p7nov requested a review from oleg-jukovec February 16, 2024 05:34
@p7nov p7nov force-pushed the gh-3725-tt-cluster branch from f2f6692 to e72ed58 Compare February 16, 2024 06:22
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you for the patch!

@p7nov p7nov linked an issue Feb 19, 2024 that may be closed by this pull request
@p7nov p7nov requested a review from andreyaksenov February 19, 2024 06:17
Copy link
Contributor

@andreyaksenov andreyaksenov left a comment

Choose a reason for hiding this comment

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

Looks cool, see my comments below:


``COMMAND`` is one of the following:

* ``publish``: publish a cluster configuration from a file.
Copy link
Contributor

Choose a reason for hiding this comment

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

from a file

Confuses me a bit because the config is published FROM a file and can be published TO a file or centralized storage. So, I'd either clarified this or simply changed to publish a cluster configuration..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified the source file definition to avoid confusion with app's config.yaml.


$ tt cluster publish myapp source.yaml

To print a local configuration from ``config.yaml`` in an application directory,
Copy link
Contributor

@andreyaksenov andreyaksenov Feb 19, 2024

Choose a reason for hiding this comment

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

Read like a local configuration is printed in an application directory on the first attempt :) Maybe, it's better to rephrase somehow to avoid confusion.


$ tt cluster show "http://myuser:p4$$w0rD@localhost:2379/myapp"

They are applied with the following precedence, from highest to lowest:
Copy link
Contributor

Choose a reason for hiding this comment

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

As an idea, list the examples above using this order and mention their precedence right in the intro to get rid of the second list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Rephrased the intro and merged the lists.


.. _tt-cluster-instance:

Managing configurations of specific instances
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, didn't know about this feature before

iproto:
listen:
- uri: 127.0.0.1:3389
threads: 10
Copy link
Contributor

@andreyaksenov andreyaksenov Feb 19, 2024

Choose a reason for hiding this comment

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

Small suggestions regarding this code snippet:

  • Rename instance.yaml -> instance_source.yaml to avoid confusion with instances.yaml.
  • Change 3389 to smth else because it is used for RDP access in Windows. And this adds unnecessary associations :) Maybe smth like 3311 or 33001 to show the difference with the default iproto port.


.. option:: -u, --username STRING

A username for connecting to the configuration storage.
Copy link
Contributor

@andreyaksenov andreyaksenov Feb 19, 2024

Choose a reason for hiding this comment

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

The embedded help tells:

username (used as etcd credentials only)

Shouldn't we mention that this is applicable to etcd only? The same for -p.

We can also think about mentioning about environment variables in the Options reference (like we do in config options).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, these descriptions in command help are outdated, I've checked with @ oleg-jukovec

URI format
~~~~~~~~~~

A URI of the cluster configuration storage has the following format:
Copy link
Contributor

Choose a reason for hiding this comment

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

An idea from the top of my head (maybe I'm wrong here): move the entire URI format section to reference and keep only How-to info here. So, the reference might include two levels:

  • Reference
    • Options
    • URI format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is generally OK but I don't see much benefit.

Authentication
~~~~~~~~~~~~~~

There are three ways to pass the credentials for connecting to the centralized configuration storage:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like they apply to etcd only? Or Credentials specified in the storage URI applies to Tarantool-config storage, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All three methods apply to both storage types. Even env variables that have ETCD in their names...

Copy link
Contributor

Choose a reason for hiding this comment

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

Even env variables that have ETCD in their names...

OMG!


A password for connecting to the configuration storage.

.. note::
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently it's hard to understand to which option this note relates. Maybe, simply add a short See also for both -u and -p.

@p7nov p7nov requested a review from andreyaksenov February 20, 2024 04:35
@p7nov p7nov merged commit fe9585d into latest Feb 20, 2024
@p7nov p7nov deleted the gh-3725-tt-cluster branch February 20, 2024 08:16
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.

new key argument for tt cluster show and tt cluster publish New tt CLI module - cluster
3 participants