feat: add config management package #108

namit-chandwani · 2021-07-01T19:25:20Z

Closes #69
Based on discussion: #100

Description

Added a config package that offers a convenient mechanism for both host and plugin developers to manage configurations in their CLIs.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

Checklist

Documentation added for the feature
CI and all relevant tests are passing
Code Review completed
Verified independently by reviewer

core/config/config.go

wtrocki · 2021-07-01T19:32:32Z

docs/src/charmil_config.md

+
+- Uses [Viper](https://github.com/spf13/viper) under the hood.
+- Helps in maintaining all available configurations in a single central-local config file.
+- Provides the plugin developers with a set of methods to maintain a configuration map and export it to the host CLI.


Wow. Very cool!

docs/src/charmil_config.md

wtrocki · 2021-07-01T19:35:16Z

docs/src/charmil_config.md

+      }
+      ```
+
+  7.  Map a plugin name to its config map using the `SetPluginCfg` method by passing the name of the plugin (as you want it in the config file) as the first argument and the config map imported from the plugin, as the second argument.


Later phase this could be automated by plugin itself

docs/src/charmil_config.md

wtrocki · 2021-07-01T19:44:25Z

docs/src/charmil_config.md

+      h.MergePluginCfg()
+      ```
+
+  9.  Write current config into the local config file using the `Save` method.


So this is approach where we migrate one time only config..

I do see it too much of hassle.

Lets put all things in the table:

some clis hqve their own config and will work with it using structure - like rhoas cli does

then cli can become plugin and have the same form of config but saved to the single file under key along with other plugins

For simplicity we assume that plugin cli was never installed on os and only host is.

What I do not get is - why host needs to interact with plugin and use non typed api (strings). We need only to abstract read and write?

My understanding was that all we need to do is to provide dynamic config location for fetching config. Some code here is going to do it but not getting the other rest.

So config of plugin can be:

entire file on it's own

some subpath of json when running in host

Missing anything?

Typed by one hand while feeding newborn. Sorry for typos

Since plugin is inside host we can also import config structures into host.

Can you reply to this?

Okay so I revisited the Rhoas config implementation and I think now I understand what the issues with my current approach are. Will change all the things that you've mentioned.

I've tried to summarize all the current issues with reasons and solutions for the same, through this comment: #108 (comment)

Please have a look and let me know if there is anything to be added/removed :)

docs/src/charmil_config.md

wtrocki · 2021-07-01T19:58:46Z

@namit-chandwani I think that I totally missunderstood your proposal from discussion. Sorry for that.

Good news is that I see some useful code that with minor adjustments will work for us.

Bad news is that I feel like you have done much more than that and it might not be needed.

This PR is very promissing so no worries.
Be prepared for +100 comments.
Finally I see something we can work on :)

namit-chandwani · 2021-07-02T18:25:42Z

Summary:

-	Issues with my current implementation	Reasons	Solutions	Status
1.	Using maps to store config in both host and plugin	Map is untyped and dynamic whereas we need a strongly typed config	Use a struct instead	Done
2.	Storing plugin config in a map and exporting it to host	Same as reason for pt.1	Allow plugins to read/write config values from/into their own set of config structs Let the plugin config to be stored under a JSON key by calling a relevant method from the plugin CLI itself	Done
3.	Host CLI developers need to merge plugin configs into the central config file	Unnecessary extra work for host CLI developers, which can be easily avoided by some changes in code	Solution for pt.2 will more or less solve this issue too	Done

wtrocki · 2021-07-05T14:38:32Z

Needs rebase. Is that ready for review?

core/config/config.go

jackdelahunt · 2021-07-05T15:29:40Z

examples/host/main.go

+}
+
+func main() {
+	// TODO: Initialize the factory struct here and pass it to the plugins


So is the idea the plugin will be able to load and save data to and from the config file with a Handler that is in factory? That we would then populate cfg with our config struct.

Yes, you're absolutely right.

Just that we won't need to pass the Handler as a whole into the factory. Instead, only including the local config file path in the factory and passing it to the plugins will suffice. WDYT?

Yeah I think your right, just storing the path would make more sense to me.

@jackdelahunt BTW just to update you, according to the latest implementation, we will be including the entire Handler in the factory (just as you had mentioned earlier).

Will create an issue/PR for the same shortly.

wtrocki · 2021-07-05T15:31:36Z

So is the idea the plugin will be able to load saved data from the config file when this gets set up? And also save?

Yes. Both read and save should work with the plugin host scenarios

jackdelahunt · 2021-07-05T15:38:18Z

Also make sure to update the docs before next review.

namit-chandwani · 2021-07-05T15:38:47Z

Needs rebase. Is that ready for review?

Just making some last-minute changes and updating docs. Will rebase and ping you once done

Also make sure to update the docs before next review.

Yes, doing that right now :)

jackdelahunt · 2021-07-05T15:44:15Z

Yes, doing that right now :)

Thanks, make sure to take your time and enjoy it. No need to put a lot of pressure on yourself just because we want to review.

namit-chandwani · 2021-07-06T10:20:39Z

Forgot to ping you earlier 😅

This is ready for review and the changes discussed in review comments have been made.

CC @wtrocki @jackdelahunt

wtrocki

Very good step forward. I left some questions about merging config. Everything else looks ok.
I need to play with this myself more once we merge it

wtrocki · 2021-07-06T10:30:59Z

core/config/config.go

+)
+
+// Handler defines the fields required to manage config.
+type Handler struct {


Too Generic name

Resolved via commit: 822770b

wtrocki · 2021-07-06T10:32:37Z

core/config/config.go

+// (using the file path linked to the handler).
+func (h *Handler) Save() error {
+	// To store the current contents of the local config file
+	dst := &map[string]interface{}{}


Any reason why we do not use structures here?

I do not question the approach but this seems to be overly large comparing to 2 liner that will save root structure
that user will provide.

Resolved via commit: 7f08531

wtrocki · 2021-07-06T11:42:04Z

docs/src/charmil_config.md

+      fmt.Println(cfg.Key6) // Prints: val6
+      ```
+
+  5.  Use the `MergePluginCfg` function to merge the current plugin config into the local config file


So far all was really good, but I feel that this merge can be done by including structure of the plugin config inside host config. This way we get serialization sorted out of the box. (Requirement: it needs to be json is fine for now)

Resolved via commit: a337275

jackdelahunt

From what I can see besides the points that have already been raised this is looking really good so far. Good work!

namit-chandwani · 2021-07-06T17:13:18Z

TODO:

Rename config handler struct
Modify the MergePluginCfg function
Modify the Save method
Update docs

wtrocki

I really hate to be this guy comming back and forth on this. I trust you have applied all suggestions. Lets merge it and refine it on main branch

wtrocki · 2021-07-06T17:28:24Z

Move todos to issues so build will pass..Or better afdress them

namit-chandwani · 2021-07-06T18:28:58Z

@wtrocki This PR is ready to merge now.
I've made all the required changes and removed the TODO comments (will create new issues for them shortly), so that build could pass.

namit-chandwani added the enhancement New feature or request label Jul 1, 2021

namit-chandwani added this to In progress in Charmil WIP via automation Jul 1, 2021

namit-chandwani requested a review from wtrocki July 1, 2021 19:25

namit-chandwani moved this from In progress to Review in progress in Charmil WIP Jul 1, 2021