refactor: Make config internal to CLI #2310

nasdf · 2024-02-13T00:47:03Z

Relevant issue(s)

Resolves #2257

Description

This PR refactors the config into a simpler implementation that exists within the cli package.

Notable changes:

~~datastore.store config key has been replaced with datastore.badger.inMemory~~
~~--store cli flag has been replaced with --in-memory~~
log.overrides config map has been added. (contains same keys as log but for individual loggers)
--logger cli flag has been temporarily removed (will be replaced in upcoming log library)
init command has been removed. (can be replaced with rm -rf ~/.defradb or similar)

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-02-13T02:44:16Z

Codecov Report

Attention: 86 lines in your changes are missing coverage. Please review.

Comparison is base (765156e) 74.12% compared to head (25c427b) 74.28%.
Report is 1 commits behind head on develop.

❗ Current head 25c427b differs from pull request most recent head 225f02e. Consider uploading reports for the commit 225f02e to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2310      +/-   ##
===========================================
+ Coverage    74.12%   74.28%   +0.17%     
===========================================
  Files          259      255       -4     
  Lines        25796    25223     -573     
===========================================
- Hits         19119    18736     -383     
+ Misses        5341     5194     -147     
+ Partials      1336     1293      -43

Flag	Coverage Δ
all-tests	`74.28% <71.43%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
cli/backup_export.go	`89.58% <100.00%> (ø)`
cli/backup_import.go	`100.00% <100.00%> (ø)`
cli/cli.go	`100.00% <100.00%> (ø)`
cli/collection_create.go	`53.57% <100.00%> (ø)`
cli/collection_delete.go	`44.44% <100.00%> (ø)`
cli/collection_describe.go	`83.78% <100.00%> (ø)`
cli/collection_get.go	`70.97% <100.00%> (ø)`
cli/collection_update.go	`42.67% <100.00%> (ø)`
cli/index_create.go	`87.76% <100.00%> (ø)`
cli/index_drop.go	`76.92% <100.00%> (ø)`
... and 29 more

... and 12 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aec7a61...225f02e. Read the comment docs.

fredcarle

A few comments while it's in draft :)

cli/config.go

cli/utils.go

cli/config.go

AndrewSisley

Looks good to me, very happy with the amount of deleted code :)

I have a couple of small comments that I would like resolving before merge, and a small concern around the below:

--logger cli flag has been temporarily removed (will be replaced in upcoming log library)

But if you and everyone else is comfy with that, I'm happy enough.

AndrewSisley · 2024-02-13T19:58:02Z

README.md

@@ -73,6 +73,8 @@ In this document, we use the default configuration, which has the following beha

 The GraphQL endpoint can be used with a GraphQL client (e.g., Altair) to conveniently perform requests (`query`, `mutation`) and obtain schema introspection.

+Read more about the configuration [here](./docs/config.md).


praise: This is nice, and keeps both .md focused.

AndrewSisley · 2024-02-13T20:00:35Z

cli/config.go

+		return err
+	}
+	err := cfg.SafeWriteConfig()
+	if _, ok := err.(viper.ConfigFileAlreadyExistsError); ok { //nolint:errorlint


todo: Can you please document the reasons for the nolint stuff with code-comments please (in all locations).

AndrewSisley · 2024-02-13T20:01:42Z

cli/config.go

+		return nil, err
+	}
+
+	// make paths relative to the rootdir


praise: Thank you for comments like this, it helps a lot when the code itself is non-obvious.

AndrewSisley · 2024-02-13T20:05:11Z

cli/config.go

+
+	var format logging.EncoderFormat
+	switch value := cfg.GetString("format"); value {
+	case "json": //nolint:goconst


suggestion: It is annoying that the linter flags this, but I think that using a const (even though that is also bad here) is slightly less bad that a nolint.

I moved all of them to const for now. They should be removed when the new logging package is ready.

AndrewSisley · 2024-02-13T20:07:41Z

cli/tx_commit.go

 			id, err := strconv.ParseUint(args[0], 10, 64)
 			if err != nil {
 				return err
 			}
-			tx, err := http.NewTransaction(cfg.API.Address, id)
+			tx, err := http.NewTransaction(cfg.GetString("api.address"), id)


question: This feels like it should be a shared const somewhere, not hardcoded here - do you have reasons for declaring it here like this?

(same for the other locations where this is done)

I wanted to avoid having const keys or a pre-defined config struct so that the config can be modified more easily. The places that access the api.address specifically should have good test coverage since they use random port assignments in the tests.

AndrewSisley · 2024-02-13T20:09:47Z

docs/config.md

+
+## `datastore.maxtxnretries`
+
+Maximum number of times to retry a failed transaction. Defaults to `5`.


todo: I don't think this line fully explains this option, from my memory this is for P2P only txn commit attempts, but that is not clear here.

I've updated it with a bit more text from the db documentation.

Cheers, new text looks good.

AndrewSisley · 2024-02-13T20:14:41Z

docs/config.md

+Store can be badger or memory. Defaults to `badger`.
+
+- badger: fast pure Go key-value store optimized for SSDs (https://github.com/dgraph-io/badger)
+- memory: in-memory version of badger


thought: memory here confuses me a little bit:

We have our own memory store, and I though that was what datastore.store=memory was

Badger memory is driven by the badger memory boolean option, if I am incorrect with (1), then why are we duplicating the option

suggestion: Unless I'm really confused/incorrect here, and given that we do document some badger config options in this file, I think it might be worth documenting the badger memory config prop.

I thought the same thing and had originally changed datastore.store to datastore.badger.inMemory, but @jsimnz had a preference to keep it datastore.store.

We could swap it to the memory datastore, but I'd prefer to do that in a new PR.

Ah fair enough.

We could swap it to the memory datastore, but I'd prefer to do that in a new PR.

I agree that if we want to make that change it should be done in another PR.

fredcarle

LGTM. Thanks for applying the changes :)

shahzadlone

LGTM, do suggest making the commit/PR Title abit more detailed rather than just Config after the commit labal

## Relevant issue(s) Resolves sourcenetwork#2257 ## Description This PR refactors the config into a simpler implementation that exists within the `cli` package. Notable changes: - ~~`datastore.store` config key has been replaced with `datastore.badger.inMemory`~~ - ~~`--store` cli flag has been replaced with `--in-memory`~~ - `log.overrides` config map has been added. (contains same keys as `log` but for individual loggers) - `--logger` cli flag has been temporarily removed (will be replaced in upcoming log library) - `init` command has been removed. (can be replaced with `rm -rf ~/.defradb` or similar) ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS

nasdf added 5 commits January 31, 2024 09:51

refactor config

daf61f6

update cli with refactored config

da779cd

Merge branch 'develop' into nasdf/refactor/config

42296e4

Merge branch 'develop' into nasdf/refactor/config

19359da

move config into cli package

9739882

nasdf added the area/config Related to configuration label Feb 13, 2024

nasdf added this to the DefraDB v0.10 milestone Feb 13, 2024

nasdf self-assigned this Feb 13, 2024

nasdf added 2 commits February 12, 2024 16:47

fix private key loading logic

6bd738a

move rootdir to cli context

4ee8d50

fredcarle reviewed Feb 13, 2024

View reviewed changes

cli/config.go Outdated Show resolved Hide resolved

cli/utils.go Show resolved Hide resolved

cli/utils.go Outdated Show resolved Hide resolved

cli/config.go Show resolved Hide resolved

nasdf added 3 commits February 13, 2024 09:03

review suggestions

de99a36

add config doc

19e428b

Merge branch 'develop' into nasdf/refactor/config

7b049a8

nasdf marked this pull request as ready for review February 13, 2024 18:06

nasdf requested review from a team and fredcarle February 13, 2024 18:06

AndrewSisley approved these changes Feb 13, 2024

View reviewed changes

review fixes

9dfb1b6

fredcarle approved these changes Feb 14, 2024

View reviewed changes

Merge branch 'develop' into nasdf/refactor/config

25c427b

shahzadlone approved these changes Feb 15, 2024

View reviewed changes

nasdf changed the title ~~refactor: Config~~ refactor: Make config internal to CLI Feb 15, 2024

Merge branch 'develop' into nasdf/refactor/config

225f02e

nasdf merged commit ae6cebe into sourcenetwork:develop Feb 16, 2024
27 of 28 checks passed

nasdf mentioned this pull request Mar 6, 2024

API.Address config value should be able to handle hostnames #2300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Make config internal to CLI #2310

refactor: Make config internal to CLI #2310

nasdf commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

fredcarle left a comment

AndrewSisley left a comment

AndrewSisley Feb 13, 2024

AndrewSisley Feb 13, 2024

AndrewSisley Feb 13, 2024

AndrewSisley Feb 13, 2024

nasdf Feb 14, 2024

AndrewSisley Feb 13, 2024

nasdf Feb 14, 2024

AndrewSisley Feb 13, 2024

nasdf Feb 14, 2024

AndrewSisley Feb 14, 2024

AndrewSisley Feb 13, 2024

nasdf Feb 14, 2024

AndrewSisley Feb 14, 2024

fredcarle left a comment

shahzadlone left a comment

		@@ -73,6 +73,8 @@ In this document, we use the default configuration, which has the following beha

		The GraphQL endpoint can be used with a GraphQL client (e.g., Altair) to conveniently perform requests (`query`, `mutation`) and obtain schema introspection.

		Read more about the configuration [here](./docs/config.md).


		## `datastore.maxtxnretries`

		Maximum number of times to retry a failed transaction. Defaults to `5`.

refactor: Make config internal to CLI #2310

refactor: Make config internal to CLI #2310

Conversation

nasdf commented Feb 13, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Feb 13, 2024 • edited Loading

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading