search

coniks-sys / coniks-go

A CONIKS implementation in Golang
http://coniks.org
Other
116 stars 30 forks source link

Fix application.LoadConfig bug #203

Closed masomel closed 6 years ago

masomel commented 6 years ago

Fixes #200

vqhuy commented 6 years ago

I gave this a try in https://github.com/coniks-sys/coniks-go/compare/gh200?expand=1, separated into 2 commits. Feel free to merge gh200 if you think it's okay.

masomel commented 6 years ago

What you did definitely works, but it seems a bit more complex than it needs to be, IMO. I definitely like the idea of having a common configuration abstraction in ConfigService, but I don't really like the seemingly cyclical use of the use of AppConfig. What I mean is the fact that the ConfigService struct includes an AppConfig field, and then each of the component Config structs "extends" a ConfigService and also implements the AppConfig interface. I'll see how I can include your changes, but I still think we don't need the AppConfig interface anymore.

vqhuy commented 6 years ago

OK, it's a fair point.

masomel commented 6 years ago

@c633, I ended up re-introducing the AppConfg interface as you suggested, but I created a separate ConfigLoader interface that allows us to support different config encodings. Please let me know what you think.

vqhuy commented 6 years ago

For now, I think introducing a new ConfigLoader interface makes things more complicated than necessary, since we've already hardcoded toml encoding everywhere and nothing else. It seems like ServerBaseConfig is used nowhere, maybe it can be safely removed?

masomel commented 6 years ago

ConfigLoader interface makes things more complicated than necessary, ...

I don't think the ConfigLoader really makes things much more complicated for developers, but I do agree that it's not really necessary at the moment, especially since we don't currently support other config encodings. But I think it's great to have for future extensibility.

...since we've already hardcoded toml encoding everywhere and nothing else.

I realized this, too. I was wondering if those hardcoded struct tags are really necessary? Much like we did for our message encoding in the protocol layer where we removed the JSON tags, we could remove the toml tags in the config encodings?

It seems like ServerBaseConfig is used nowhere, maybe it can be safely removed?

Yep, especially now that we want to remove EpochDeadline from SeverBaseConfig we can remove it.

vqhuy commented 6 years ago

Great! LGTM!