Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi level Hocon config #254

Merged
merged 3 commits into from
Aug 12, 2014
Merged

Multi level Hocon config #254

merged 3 commits into from
Aug 12, 2014

Conversation

rogeralsing
Copy link
Contributor

  • Moved HOCON tests
  • Implemented multi level hocon test
  • Hocon Parser now returns a HoconRoot , (HoconValue and a list of HoconSubstitution)
  • Fluent multi level Fallback is now supported

Breaking change!

ActorSystem.Create("name") now calls ConfigurationFactory.Load() which in turn gets the config from App/Web.config
Earlier versions only used Pigeion.config if no config was supplied.
This change aligns more to Akka.

This only affects systems created w/o a config, effect for real users should be minimal

ActorSystem.Create("name", config) still behaves as before.

* Implemented multi level hocon test
var config3 = ConfigurationFactory.ParseString(hocon3);
var config4 = ConfigurationFactory.ParseString(hocon4);

var config = config1.WithFallback(config2.WithFallback(config3.WithFallback(config4)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ se above, creating multi level hocon

Problems possibly arise if one would do:

config1.WithFallback(config2).WithFallback(config3).WithFallback(config4)
since that would create an incorrect order of fallbacks.
config1 would have config2 as fallback
that new config would have config3 as fallback
Here is where things break down, since config2 does not know about config3.
This is most likely the cause of multi level hocon we have had before.

or?

cc. @Aaronontheweb

Copy link
Member

Choose a reason for hiding this comment

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

@rogeralsing one of your tests should include the original Pigeon.conf as the implicit final fallback and test to make sure that a value not present in any of the user-defined confs still appears there.

Copy link
Member

Choose a reason for hiding this comment

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

@rogeralsing yeah, that's probably where the issue is.

Here's where the real problem occurs then: https://github.com/akkadotnet/akka.net/blob/dev/src/core/Akka.Remote/RemoteActorRefProvider.cs#L30
https://github.com/akkadotnet/akka.net/blob/dev/src/core/Akka/Actor/Settings.cs#L21

Using var config = config1.WithFallback(config2) as an example here, the WithFallback on either of those two lines effectively wipes out config2 from the user's application.

So either we need to change WithFallback to append a new config to the end of the linked list (which would make config1.WithFallback(config2).WithFallback(config3).WithFallback(config4) work), or add a new method that explicitly does that and change these two calls to use it instead. WithFinalFallback or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, WithFallback should be changed to work in a fluent way, that is what one would expect from it.

* Hocon Parser now returns a `HoconRoot` , (`HoconValue` and a list of `HoconSubstitution`)

## Breaking change!

`new ActorSystem("name")` now calls `ConfigurationFactory.Load()` which in turn gets the config from App/Web.config
Earlier versions only used Pigeion.config if no config was supplied.
This change aligns more to Akka.
@rogeralsing
Copy link
Contributor Author

cc. @Aaronontheweb fluent multilevel hocon config
All tests pass.
Fixed a bug caused by test-transport beeing enabled in the Pigeon.config which spawned TestTransports w/o any local-address. that must have been there for quite a while now..

@Aaronontheweb
Copy link
Member

Good deal!

Aaronontheweb added a commit that referenced this pull request Aug 12, 2014
@Aaronontheweb Aaronontheweb merged commit aaef770 into akkadotnet:dev Aug 12, 2014
Aaronontheweb referenced this pull request in Aaronontheweb/akka.net Aug 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants