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

Fix Provider updated_by_last_action return value #168

Closed
wants to merge 1 commit into from

Conversation

phlipper
Copy link
Contributor

This PR uses the return value from the executed resource vs. hard-coding a true/false return.

@phlipper phlipper force-pushed the provider-notification branch from b6c6a34 to 9b641bd Compare February 5, 2015 18:41
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9b641bd on phlipper:provider-notification into b2824ae on DataDog:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9b641bd on phlipper:provider-notification into b2824ae on DataDog:master.

@miketheman
Copy link
Contributor

This commit does a lot more than update the action, it changes style as well.
Not saying that the change isn't welcome, but sectioning them to behavior vs style changes is a better approach, as they can be diff'ed accordingly.

@phlipper
Copy link
Contributor Author

phlipper commented Feb 5, 2015

@miketheman are you asking me to change this PR now, or are you just commenting generally?

@miketheman
Copy link
Contributor

I think I'm commenting in general. i've been working through a bunch of bug tickets here, and commenting on issues as I notice things.
I may split & rebase the code so that it does that when I get to this feature.

@miketheman miketheman added this to the Next minor milestone Aug 19, 2015
@olivielpeau
Copy link
Member

@phlipper Unfortunately this approach doesn't change the current behavior because t.updated_by_last_action? is evaluated during the compile phase and therefore always evaluates to false. For reference see chef/chef#3748.

A comment on the above issue recommends using use_inline_resources instead, but it has the drawbacks of only working on Chef >= 11 and of not allowing external resources to be notified inside of the LWRP (see https://docs.chef.io/lwrp.html#disable). As the behavior of use_inline_resources will be the default one in future versions of Chef we should think about handling it though (maybe by notifying the datadog-agent service when datadog_monitor is used, or by using definitions).

Also, for reference, this email on the chef mailing list recommends running the embedded resource of the LWRP before evaluating updated_by_last_action? on it, but this approach has the same drawback of not allowing to notify external resources in the embedded resource.

@miketheman Any thoughts on this?

@miketheman
Copy link
Contributor

@olivielpeau The current cookbook aims to support Chef 10, so a pattern used there is:

use_inline_resources if defined?(use_inline_resources)

This pattern dates back a while.

So I think the final desire could be accomplished using the inline resources approach, as long as it's qualified.

There's also this post on the approach.

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

Successfully merging this pull request may close these issues.

4 participants