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

Add on_failure handler (API) #1

Merged
merged 1 commit into from Jul 10, 2014
Merged

Add on_failure handler (API) #1

merged 1 commit into from Jul 10, 2014

Conversation

@sethvargo
Copy link

@sethvargo sethvargo commented Dec 10, 2013

Pretty render

@adamhjk
Copy link
Contributor

@adamhjk adamhjk commented Dec 12, 2013

+1 - this looks awesome.

@marcparadise
Copy link
Contributor

@marcparadise marcparadise commented Dec 12, 2013

This looks really cool. I do have one concern around a possible antipattern: given that the reference to the parent resource is provided, would there be any sane method by which someone could simple remove that resource or otherwise indicate that it converged successfully, causing it to skip convergence and yielding a final state that's not -quite-as-declared ?

@ranjib
Copy link
Contributor

@ranjib ranjib commented Dec 12, 2013

👍 we currently use a exception to introspect the failed resources, this will greatly reduce the pain.

@sethvargo
Copy link
Author

@sethvargo sethvargo commented Dec 12, 2013

@adamhjk thanks!

@marcparadise probably? But you can also do that now by mucking through the resource_collection and the run_context. @stevendanna and I were talking about the best thing for that block to yield, and it seemed like the resource would be best. That being said, I'm totally open to other ideas if you got 'em 😄!

@ranjib that pager_duty resource though 😄

@thoughtcroft
Copy link

@thoughtcroft thoughtcroft commented Dec 12, 2013

+1 this will be super useful and help avoid some messy hacks

@buildscientist
Copy link

@buildscientist buildscientist commented Dec 12, 2013

+1 - great idea.

How does this tie into existing report handlers? I suppose one could always notify the report handler.

Additionally - I'm assuming that this would override (or replace) any existing exception handlers?

```ruby
meal 'breakfast' do
on_failure do |breakfast|
alarm breakfast.start_time do
Copy link
Contributor

@coderanger coderanger Dec 12, 2013

Why not just run the block as an instance_exec against the provider object?

Copy link
Author

@sethvargo sethvargo Dec 12, 2013

Because you probably don't want the provider, you want the resource. And you may want the entire run_context as well.

If we just evaluate the block it will simply add the resource to the resource_collection and not run any action. Is that the expected behavior?

@stevendanna
Copy link
Contributor

@stevendanna stevendanna commented Dec 12, 2013

I'm 👍 I think this covers everything we talked about and then some.

@caryp
Copy link

@caryp caryp commented Mar 11, 2014

I wish I could use this today. Very cool, @sethvargo.

meal 'breakfast' do
on_failure { notify :eat, 'food[bacon]' }
end
```

How about allowing some attributes to be overridden for the notifying resource?
For example:

meal 'breakfast' do
  on_failure { notify :eat, 'food[bacon]', quantity: 3 }
end

This can override the attribute(s) set on the existing food[bacon] resource before executing the action.

Copy link
Author

@sethvargo sethvargo Mar 18, 2014

Why? What is quantity? There's already a "retries" key below

quantity is an attribute to the food resource.

Copy link
Author

@sethvargo sethvargo Mar 18, 2014

Nah, I'm 👎 on that. If you need to modify the resource, there's something wrong. You also can get the resource handle and change it manually if you wanted. But I don't like this syntax.

@jblaine
Copy link

@jblaine jblaine commented Apr 9, 2014

If there's a way, it would be worthwhile to indicate some more information in the failure log line:

# At least 1 bacon slice is required to complete breakfast without getting hungry.
node.override['meal']['bacon_required'] = 1

meal 'breakfast' do
  on_failure(HungryError) { notify :eat, 'food[bacon]' }
end

food 'bacon' do
  action :nothing
end
[2014-04-09T04:07:12+00:00] INFO: meal[breakfast] failed with:
#<MealExceptions::HungryError: I want 1 bacon slices. But I only ate: 0>
[2014-04-09T04:07:12+00:00] INFO: Ate 'bacon' successfully. yummy!

As in:

[2014-04-09T04:07:12+00:00] INFO: meal[breakfast] failed with:
#<MealExceptions::HungryError: I want 1 bacon slices. But I only ate: 0>. AND
BECAUSE OF THAT, I AM DOING XYZ
[2014-04-09T04:07:12+00:00] INFO: Ate 'bacon' successfully. yummy!

@sethvargo
Copy link
Author

@sethvargo sethvargo commented Apr 9, 2014

It looks like @arangamani did some awesome work and turned this RFC into a cookbook: https://github.com/arangamani-cookbooks/on_failure

It follows this RFC to a T and looks pretty delightful to me. I encourage everyone to try it out! 😄 Great work!

/cc @adamhjk @marcparadise @ranjib @thoughtcroft @buildscientist @coderanger @caryp @stevendanna @paulmooring

@adamhjk
Copy link
Contributor

@adamhjk adamhjk commented Jul 9, 2014

I say we should merge this request, and snarf the cookbooks implementaiton in to core chef.

👍 - who agrees?

@sethvargo
Copy link
Author

@sethvargo sethvargo commented Jul 9, 2014

👍

@arangamani
Copy link

@arangamani arangamani commented Jul 9, 2014

👍

1 similar comment
@douglaswth
Copy link

@douglaswth douglaswth commented Jul 9, 2014

👍

@stevendanna
Copy link
Contributor

@stevendanna stevendanna commented Jul 9, 2014

👍

1 similar comment
@caryp
Copy link

@caryp caryp commented Jul 9, 2014

+1

adamhjk added a commit that referenced this issue Jul 10, 2014
@adamhjk adamhjk merged commit 8470f63 into master Jul 10, 2014
@sethvargo sethvargo deleted the sethvargo/on_failure branch Jul 10, 2014
nathenharvey added a commit that referenced this issue Sep 18, 2014
@athingisathing
Copy link

@athingisathing athingisathing commented Jul 6, 2016

When will this be in prod?

@cnmcavoy
Copy link

@cnmcavoy cnmcavoy commented Apr 5, 2017

Whatever happened to this RFC? It looks like it was accepted, but never moved past the proof-of-concept stage.

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