Skip to content
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

Add comments to documentation #22

Closed
imclint21 opened this issue Nov 18, 2019 · 4 comments
Closed

Add comments to documentation #22

imclint21 opened this issue Nov 18, 2019 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed proposition
Milestone

Comments

@imclint21
Copy link
Member

imclint21 commented Nov 18, 2019

I think it could be cool to have comments in documentation!

# When diskless replication is used, the master waits a configurable amount of
# time (in seconds) before starting the transfer in the hope that multiple replicas
# will arrive and the transfer can be parallelized.
#
# With slow disks and fast (large bandwidth) networks, diskless replication
# works better.
repl-diskless-sync no

Issue Opened on serde_yaml Github: dtolnay/serde-yaml#145

@imclint21 imclint21 added the enhancement New feature or request label Nov 18, 2019
@imclint21 imclint21 added this to the pre-alpha milestone Nov 18, 2019
This was referenced Nov 25, 2019
@shuni64
Copy link
Contributor

shuni64 commented Nov 29, 2019

While I like this idea, I'm not sure if this is easily possible to do in code, even if it is it doesn't seem like a good idea to hardcode documentation as strings in the binary.

Instead I suggest keeping a default configuration file with comments in the repository and packaging it with releases. The authentication tokens and keys would have to be stored somewhere else, but maybe this should be the case anyway.

This also gives us the opportunity rethink the idea of explicitly initializing the configuration file through lucid. If a default configuration is packaged with releases there shouldn't be a need to create one when running Lucid for the first time. The only thing that should have to be generated is authentication data, because it doesn't have a good default.

I also had another thought when looking at the way configuration is handled: The server and the interactive cli are the same binary.
Maybe this should not be the case, and all settings changes by the cli should have to go through appropriate api endpoints on the server.
This way the server and the cli are decoupled and don't have to run on the same machine, which has long-term benefits like being able to temporarily change a setting without restarting the server and allowing other programs to change settings with api requests.
It would also clean up the code quite a bit, as most of the cli handling would be gone from the server.

@imclint21 imclint21 added help wanted Extra attention is needed proposition labels Nov 29, 2019
@imclint21
Copy link
Member Author

imclint21 commented Nov 29, 2019

It's a good idea on the paper, just some points are important.

Lucid is build to be a DevOps software, running with some Linux standards, with /etc/lucid/lucid.yml for configuration and /var/log/lucid.yml for logging by example.

So pack a default configuration file in the release zip is not a bad idea but we also need to keep the init command to have both possibilities in my opinion.

If we deploy Lucid on Linux package managers maybe we could add default configuration with comments and not in the GitHub releases.

The init command is important for me to bootstrap the Lucid instance very quickly, some programs like consul etc don't have this and it's really boring to configure it.


About the CLI, I want to keep a single binary BUT you right on many points!

Firstly, if you look at this line, the actual CLI is thought (but not coded) as a way to manage lucid nodes, not only the local node but each node, maybe we could make a new API endpoint like /api/settings/ to make hot changes on the node (and also adding CLI commands)

@shuni64
Copy link
Contributor

shuni64 commented Nov 29, 2019

Keeping the init subcommand is fine, it's just that comments won't be included when the configuration is written unless the default configuration file is compiled into the binary with include_str!, but even then any changes to it would have to be written without comments.
We could just not have an api to permanently change the configuration on disk and require the user to change the files themselves, which should be fine.


Splitting Lucid into server and cli binaries isn't necessary to implement any feature, but would decouple two parts of Lucid that shouldn't directly interact with each other (only using api endpoints), so it's mainly for code quality reasons. I think I'll test some of this on my fork to see if it would bring significant enough improvements to be worth it.

@imclint21
Copy link
Member Author

The serde_yaml guys don't want to implement comments support in the lib, and a hardcoded configuration file is definitely a bad idea, so I will close this issue I think!

For the CLI and the server decoupled, I'm really for it and it's the idea [from the begining], in fact, the CLI is only an HTTP client.

Feel free to achieve this, If you use an HTTP library maybe you could don't use reqwest, it's a bit heavy (~4 MB more for the binary)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed proposition
Projects
None yet
Development

No branches or pull requests

2 participants