Skip to content
This repository has been archived by the owner on Dec 9, 2023. It is now read-only.

Fix ignored config params #110

Merged
merged 4 commits into from
Nov 28, 2020
Merged

Conversation

zoedberg
Copy link
Contributor

closes RGB-WG/rgb-sdk#19 and #102

the solution is not very elegant, but since @dr-orlovsky said:

Shared parts can be moved into a separate shared config like I did in LNP Node, but this is not a big priority for now and will be done anyway when I will be refactoring RGB Node to the best practices I used in LNP Node (there is a lot of other stuff which can be improved/made more efficient in this regard)

I tried to avoid changing the existing structure as much as possible

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to avoid unwrap (see comment), otherwise will work for now

src/i9n/runtime.rs Outdated Show resolved Hide resolved
@zoedberg
Copy link
Contributor Author

FYI I've added a commit (i9n config: add fungible|stash pub endpoints) which is needed in order to fix the rgb-node side of issue RGB-WG/rgb-sdk#14 (problem n.2). Given the commit is related to other ignored config params, I though this was the proper place to insert it

@Kixunil
Copy link

Kixunil commented Nov 27, 2020

Randomly passing by and looking at that change, I should probably try to improve configure_me so that it can be used instead. If you're interested, let me know which features of clap you need that are not in configure_me yet.

@dr-orlovsky
Copy link
Member

@Kixunil we need multiple binaries support and sectioned configuration (that allows storing multiple daemon configs under the same file)

@Kixunil
Copy link

Kixunil commented Nov 28, 2020

Multiple binaries are already supported, do you mean some specific feature related to multiple binaries?
I will try to think about a sane approach to solve sectioned configuration.

@dr-orlovsky
Copy link
Member

Yes, I meant multiple sections related to multiple binaries

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Nov 28, 2020
@dr-orlovsky dr-orlovsky added this to the v0.2 milestone Nov 28, 2020
@dr-orlovsky dr-orlovsky merged commit 4f696e5 into RGB-WG:master Nov 28, 2020
@Kixunil
Copy link

Kixunil commented Nov 28, 2020

What's the advantage of this approach over having the files separately? I see at least one disadvantage: will be harder to atomate in deployments like my Deb repository.

@dr-orlovsky
Copy link
Member

Which files? Config or binaries?

@Kixunil
Copy link

Kixunil commented Dec 10, 2020

Having a single config file for all binaries vs having a separate config file for each binary. Note that sharing common options in a single config and different options in a different config is already possible. (A clean approach IMO. Similar to include statement but without the complicated logic of include statements.)

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Dec 10, 2020

From a maintainability perspective single config covering all binaries is more efficient. In fact very few options are binary-specific, most of them are shared, so it will be strange to have a six config files most of which will have a single or two lines of configuraiton

@Kixunil
Copy link

Kixunil commented Dec 10, 2020

Yeah, this seems to be true for manual maintenance. Automated is easier with multiple files as changes to one could only affect one binary and restarting all binaries may be excessive. Multiple files avoid this problem as well as writing complicated logic to detect such situations.

Ideally we would support both. I think it can be done by each binary parsing a config and also a section of appropriate name, ignoring the rest. It would also make sure for the section to override global settings. This way this will be possible:

foo = 42

[binary1]
# overrides foo above for this binary
foo = 47

[binary2]
bar = "satoshi"

nginx works very similarly with location sections. I think this is a nice feature and (perhaps surprisingly) easier to implement it than to not implement it. There's only one obvious limitation with this approach: binary and option/switch can't share a name, but I don't think it's a huge problem. (Maybe a feature, actually?)

What do you think?

@dr-orlovsky
Copy link
Member

I think this will work. But can we have secion names matching binary names?

@Kixunil
Copy link

Kixunil commented Dec 11, 2020

I think at least first version will make section names matching binary names mandatory. :)
The question is what if someone renames the binary after building - should the name change too or shouldn't? (I think it shouldn't but if you have arguments for why it should please let me know.)

@dr-orlovsky
Copy link
Member

I agree that it should not

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignored config parameters
3 participants