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.

Sign up for GitHub

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

Jump to bottom

Use MiqRegion#role_active? method over private method on Provider #15094

Merged
merged 1 commit into from
May 15, 2017
Merged

Use MiqRegion#role_active? method over private method on Provider #15094

merged 1 commit into from
May 15, 2017

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented May 15, 2017 •
edited
Loading

🔥 ✂️ 🔥 ✂️ 🔥 ✂️ 🔥 ✂️ 🔥 ✂️

Sorry @syncrou, I didn't know about this method when you were writing this originally 😅
(Original PR: #15045)

@carbonin carbonin requested a review from syncrou May 15, 2017 17:44
@carbonin carbonin mentioned this pull request May 15, 2017
Merged
syncrou
syncrou approved these changes May 15, 2017
View reviewed changes
Copy link
Contributor

@syncrou syncrou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@carbonin - Looks good to me. I was a bit surprised no one had needed a helper method like role_enabled? before - This makes a lot more sense.

Though the tests are going to have to be cleaned up a bit.

@carbonin carbonin force-pushed the use_region_role_active_method branch from 784c80c to 5e1e9eb Compare May 15, 2017 21:41
@miq-bot
Copy link
Member

miq-bot commented May 15, 2017

Checked commit carbonin@5e1e9eb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@bdunne bdunne merged commit b5a918d into ManageIQ:master May 15, 2017
@bdunne bdunne added this to the Sprint 61 Ending May 22, 2017 milestone May 15, 2017
@carbonin carbonin deleted the use_region_role_active_method branch May 18, 2017 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syncrou syncrou syncrou approved these changes

@bdunne bdunne bdunne approved these changes

Assignees

@bdunne bdunne

Labels
providers/automation technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@carbonin @miq-bot @bdunne @syncrou @Fryguy