moogar0880 / venom

A pluggable configuration management library with zero dependencies
MIT License
66 stars 4 forks source link

Delete and Move APIs #12

Open moogar0880 opened 5 years ago

moogar0880 commented 5 years ago

The purpose of this issue is to expand upon the existing ConfigStore API to add methods for removing and renaming keys in the ConfigStore.

API

Currently there is no mechanism for performing these operations within venom, however, the logic to implement them should be fairly simple. Two new methods should be added to the ConfigStore interface:

func Move(from, to string) error
func Delete(key string)

Both of these methods must be implemented by the Venom type, which is itself a ConfigStore, along with one additional method:

func MoveAndAlias(from, to string) error

This MoveAndAlias method will, just like it sounds, perform a Move and an Alias to ensure that the configuration data is available under the new keyspace, but that any applications using the old name will still be supported.

Additional Considerations

Note: The addition of the Move and Delete methods to the ConfigStore API is a breaking change and will require a major version bump once merged.

ilmanzo commented 5 years ago

that's an interesting idea. Should "Move" work across multiple levels ? e.g.

Move("foo.bar","foo") 
Move("foo","foo.bar")
Move("foo","foo.bar.baz")

in the latter case, should necessary intermediate sub-levels be created ?

moogar0880 commented 5 years ago

Should "Move" work across multiple levels?

That's a good question, and I guess it depends what you mean by "levels". If "levels" of nested maps in the ConfigStore, then yeah I think so, otherwise you'd have to recursively move all of the keys under a root to truly move a config. For instance given a config that looks more or less like this:

{
    "foo": {
        "bar": {
            "baz": "https://example.com",
            "bat": "https://google.com"
        }
    }
}

A Move("foo.bar.baz", "foo") would result in

{
    "foo": "https://example.com"
}

However, if by "levels" you meant ConfigLevels in the heap, then that isn't something that'd I'd actually considered, but I think the answer is also yes. Which means we would effectively have to iterate over all of the config levels in the heap, check to see if the key to move has been set in that level or not and, if it has, then move it to the new key.

in the latter case, should necessary intermediate sub-levels be created ?

Yeah, I think they'd have to be, otherwise we wouldn't be able to traverse the config map properly to do a lookup or future inserts. Luckily setNested should handle that when the value is re-inserted under the new key.