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

Module's config references should refer back to the base config loaded by Halibot core #88

Open
richteer opened this issue Oct 4, 2017 · 5 comments

Comments

@richteer
Copy link
Contributor

richteer commented Oct 4, 2017

Currently, when module is instantiated, it is handed a dictionary blob of configuration data that it stores as self.config. However, this is a copy of the data contained in the config.json file loaded by Halibot on start up. Any changes to either Halibot's copy, or the module's copy, are not shared.

This can be fixed by converting the self.config reference on modules (All HalObjects, actually) into a dynamic attribute that on getattr parses Halibot's config for the relevant blob. This way, the current usage will be exactly the same to module developers, the only difference is that changes to config will be reflected back into the config file.

Note: the conf argument to a HalObject's __init__ may be useless now.

@richteer
Copy link
Contributor Author

Turns out this has some problems. Tests fail since they use temporary modules that have no config, therefore the agent's call to .connect() fails to update the non-config.

I have two thoughts on this:

  • Merge module-instances and agent-instances to object-config. Define separate config keys loaded-modules and loaded-agents or whatever that is a list of the names (as defined in object-config) that are to be loaded.

  • Continue to allow objects to use their own configs, and include an API call to sync configs back to Halibot's central config.

The former is obviously more complex and invasive. However, it does mean that all config is one neat location, and there is now a separation between object configuration, and what objects are actually meant to be loaded. This separation might be useful for debugging or testing, or something. Maybe even templates.

The latter option, is easily the simplest, though it will require agent/module devs to actively call that whenever they need to flush their config back, OR a config editing module to do that.

I think I'm actually in favor of separating the config keys (as an unrelated change), but keeping object config local. There might be cases where it will be useful to instantiate a temporary module without needing to define a key in the Halibot config for it (especially testing).

@richteer
Copy link
Contributor Author

Additional (beneficial?) side effect: Loading modules separate from config will also (potentially) allow a deterministic load order -- dictionaries have a non-deterministic key order, but a list will not. Therefore, we could (potentially) allow for config directives or other magic that handle "load-after":"<package/object>" in the case of objects that should be loaded sooner/later in the startup.

@richteer
Copy link
Contributor Author

Okay, another wild thought. We can define named configs, and therefore reuse configs for different instances.

Example:

{
    "object-configs": {
        "foo": {
            "of":"foobar:Default",
            "some-config":"yes"
        }
    },
    "load-modules": [
        "bar:foo",
        "baz:foo"
    ]
}

In this example, the config for the module foobar:Default is defined once (named foo), but used in two different instances named bar and baz. Therefore, there are two running modules, each with their own state.

This does raise a question though of if either module actually "owns" the config. That is, if one module wants to write back its config, should it write to a new config instance (which means we need to change the load-modules declaration), or should it update the same config both use?

@sjrct
Copy link
Member

sjrct commented Oct 22, 2017

I don't see how merging agent-instances and module-instances fixes the problem of not have a config in the tests. Also, I do not think that we should merge them until after a change to routing such as the one we talked about before.

@richteer
Copy link
Contributor Author

This is almost a completely unrelated proposal now. I'm arguing that modules should have complete ownership over their config (config is local to their class instance, like how it is now), and configuration should be separate from the list of modules to load. Tests should be fine with this, since they don't rely on a callback to ._hal anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants