Added config states #36

Merged
merged 1 commit into from Feb 17, 2016

Conversation

Projects
None yet
3 participants
Owner

johnsca commented Feb 16, 2016

Now you can detect config value changes with states:

@when('config.changed')
def any_config_changed():
    pass

@when('config.changed.my-option')
def my_option_changed():
    pass
Owner

marcoceppi commented Feb 16, 2016

This looks great, will review and test later today

Owner

johnsca commented Feb 16, 2016

@marcoceppi Some changes made.

@johnsca johnsca referenced this pull request in juju-solutions/layer-apache-hive Feb 16, 2016

Merged

Reconfigure service on config changes #7

Contributor

mbruzek commented Feb 16, 2016

Is there any reason this was not handled by another decorator? What happens if there is a "config.changed" state? Or if the configuration option has a dot in it?

Contributor

mbruzek commented Feb 16, 2016

Don't get me wrong, I WANT a function like this, but it seems arbitrary to base it on the string state that could collide with legit states.

Owner

marcoceppi commented Feb 16, 2016

All states should be prefixed with that layer name. As such there shouldn't
be collisions

On Tue, Feb 16, 2016, 4:27 PM Matt Bruzek notifications@github.com wrote:

Don't get me wrong, I WANT a function like this, but it seems arbitrary to
base it on the string state that could collide with legit states.


Reply to this email directly or view it on GitHub
#36 (comment)
.

Contributor

mbruzek commented Feb 16, 2016

Then shouldn't it be basic.config.changed ?

Owner

marcoceppi commented Feb 16, 2016

Basic isn't really a layer less than it is a a primitive for other layers
to build on top of so pretend this is the basic and configure the earliest
conflict. Changed this move makes it so that future events are modeled
better in a stateful way

On Tue, Feb 16, 2016, 5:04 PM Matt Bruzek notifications@github.com wrote:

Then shouldn't it be basic.config.changed ?


Reply to this email directly or view it on GitHub
#36 (comment)
.

Contributor

mbruzek commented Feb 16, 2016

I also have a use case where I want to know if config A or B or C changes. I don't think we have a way to do that with this way of reusing @When

Contributor

mbruzek commented Feb 16, 2016

States are arbitrary and technically developers can pick what ever they want. If they happen to pick "config.changed" as a state their layer emits, this breaks the functionality of the "if configuration changes". This feels to me more like a new decorator is needed @when_any_changed('A', 'B', 'C') or @when_changed('A') rather than an arbitrary state that could actually collide with the "config.changed" state.

Much like "reserved words" in programming languages. If we do this change, we need to define "reserved states" in a document somewhere so people don't shoot themselves in the foot. Reserved states still feels arbitrary and non-deterministic to me but as @johnsca explained to me it is reusing facilities that already work and have been testsed.

Owner

johnsca commented Feb 16, 2016

If a layer is choosing config.changed as their state name, it's rather likely that they're driving it off of config options anyway, in which case is it really a conflict?

A @when_any decorator has been requested before and I think is a separate issue.

Owner

marcoceppi commented Feb 17, 2016

⛵️ lgtm

marcoceppi added a commit that referenced this pull request Feb 17, 2016

@marcoceppi marcoceppi merged commit c5c949e into juju-solutions:master Feb 17, 2016

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