Explicit config creation #543
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
==========================================
- Coverage 76.83% 76.26% -0.58%
==========================================
Files 106 106
Lines 10049 9916 -133
Branches 1658 1628 -30
==========================================
- Hits 7721 7562 -159
- Misses 1796 1833 +37
+ Partials 532 521 -11 ☔ View full report in Codecov by Sentry. |
|
To provide some context about the other commands that make configs:
Both of these provide a shortcut to creating a config and filling in some values. Code wise this looks fine, my only comment is that the three separate config loaders functions feels like they could be a static method on the You will also need to update the |
|
I could not find any logic errors in the code you have written. There are a few bits of lint found by CI but other than that your changes make the repetition of the loader functions very clear. Perhaps it can be moved into a static as Hornwitser has stated, such as:
|
|
I've checked the documentation. All the config creation goes through the create script (host, controller), or through the bootstrap command (ctl). The only exception is in the 'managing-a-cluster.md' where it uses the ctl to create a host. |
| if (await fs.exists(configPath)) { | ||
| logger.error(`Config "${configPath}" already exists`); | ||
| return; | ||
| } | ||
| await libFileOps.safeOutputFile(configPath, JSON.stringify(emptyConfigSupplier(), null, "\t")); |
There was a problem hiding this comment.
Rather than using fs.exists you should use a try catch for EEXIST. Also as the happy path here is that the file does not exist, and because the config file is quite small, it is better to use fs.outputFile rather than safeOutputFile since the safe version is designed to overwrite existing files that can't risk being corrupted.
Preparation for a command that creates a config. Such command should not fail when there is no existing config (that is the point after all). However, other config commands should be able to load the config. Hence, the handler of config commands needs to be able to load the config if needed.
The three config loading methods were basically identical, only differing in: - The log message refering to the type of config (which the user probably already knows). - What function is called for parsing the json into a config object. The former difference is removed, the latter is now passed as function.
Laar
force-pushed
the
explicit-config-creation
branch
from
b3ca721
to
d1f6c50
Compare
This changes the behaviour when starting a controller, host or ctl when the config file can not be found. The intention (according to the backlog) is to prevent accidentally starting in the wrong directory.
Specifically it adds the following
config createcommand is added to create configs. This fails if the config file already exists.Thoughts
The create-config command creates a basic config for all three types (controller, host and ctl). There are already two commands to create configs,
ctl create-hostfor creating host configs andcontroller bootstrap create-ctl-configfor ctl configs. This duplication seems rather suspect.Todo