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

Conversation

Projects
None yet
@sethvargo

sethvargo commented Dec 10, 2013

@adamhjk

This comment has been minimized.

Contributor

adamhjk commented Dec 12, 2013

+1 - this looks awesome.

@marcparadise

This comment has been minimized.

Member

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

This comment has been minimized.

Member

ranjib commented Dec 12, 2013

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

@sethvargo

This comment has been minimized.

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

This comment has been minimized.

thoughtcroft commented Dec 12, 2013

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

@buildscientist

This comment has been minimized.

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

This comment has been minimized.

@coderanger

coderanger Dec 12, 2013

Contributor

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

This comment has been minimized.

@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.

This comment has been minimized.

@arangamani

arangamani Mar 18, 2014

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

This comment has been minimized.

Member

stevendanna commented Dec 12, 2013

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

@caryp

This comment has been minimized.

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
```

This comment has been minimized.

@arangamani

arangamani Mar 18, 2014

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.

This comment has been minimized.

@sethvargo

sethvargo Mar 18, 2014

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

This comment has been minimized.

@arangamani

arangamani Mar 18, 2014

quantity is an attribute to the food resource.

This comment has been minimized.

@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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

adamhjk commented Jul 9, 2014

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

👍 - who agrees?

@sethvargo

This comment has been minimized.

sethvargo commented Jul 9, 2014

👍

@arangamani

This comment has been minimized.

arangamani commented Jul 9, 2014

👍

1 similar comment
@douglaswth

This comment has been minimized.

douglaswth commented Jul 9, 2014

👍

@stevendanna

This comment has been minimized.

Member

stevendanna commented Jul 9, 2014

👍

1 similar comment
@caryp

This comment has been minimized.

caryp commented Jul 9, 2014

+1

adamhjk added a commit that referenced this pull request 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 pull request Sep 18, 2014

@athingisathing

This comment has been minimized.

athingisathing commented Jul 6, 2016

When will this be in prod?

@cnmcavoy

This comment has been minimized.

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