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

Convert actions in Chef::Resource::Notification to symbols to prevent double notification #6515

Merged
merged 4 commits into from Jan 22, 2018

Conversation

dimsh99
Copy link
Contributor

@dimsh99 dimsh99 commented Oct 23, 2017

Signed-off-by: dmitrys dmitrys@northernlight.com

Description

Prevents notifying resources twice if notifications are created with action as String and as Symbol

Issues Resolved

#6242

Check List

Signed-off-by: dmitrys <dmitrys@northernlight.com>
@dimsh99 dimsh99 requested a review from a team October 23, 2017 19:57
@dimsh99
Copy link
Contributor Author

dimsh99 commented Oct 26, 2017

@thommay I believe CI failures are not connected to code changes. Actually, lately travis-ci runs fail and pass on the same code base sporadically.

action.to_sym
else
action
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just call action.to_sym here. Symbol#to_sym is just a no-op

Copy link
Contributor Author

@dimsh99 dimsh99 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I first did. Then some unit tests failed because action was set to nil in some of them. So I decided to check if value of action actually supports to_sym

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use action&.to_sym.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thank you. Will do.

Signed-off-by: Dmitry Shestoperov <dmitry@shestoperov.info>
@tas50
Copy link
Contributor

tas50 commented Nov 29, 2017

@dimsh99 can you rebase this to pull in some CI fixes we've rolled out

@dimsh99
Copy link
Contributor Author

dimsh99 commented Nov 29, 2017

@tas50 thank you. Travis tests passed. There are still some AppVeyor's failures though.

@tas50 tas50 changed the title action of Chef::Resource::Notification gets converted to symbol if possible Convert actions in Chef::Resource::Notification to symbols to prevent double notification Dec 19, 2017
@lamont-granquist lamont-granquist merged commit 56568c0 into chef:master Jan 22, 2018
@lock
Copy link

lock bot commented May 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants