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

Enable custom actions for Vms API #14817

Merged
merged 4 commits into from
Apr 26, 2017

Conversation

imtayadeway
Copy link
Contributor

As required by https://www.pivotaltracker.com/n/projects/1914499/stories/142774757

/cc @jjlangholtz

Haven't tested this extensively, just submitting for feedback. @jjlangholtz feel free to pull down and kick the wheels

@miq-bot add-label api, enhancement
@miq-bot assign @abellotti

@abellotti
Copy link
Member

Nice work @imtayadeway on the model side 🎵 I'll review shortly.

@imtayadeway imtayadeway force-pushed the api/vm-custom-actions branch from 3e95b04 to 663ce76 Compare April 19, 2017 17:57
@jjlangholtz
Copy link
Contributor

@imtayadeway Pulled down and it looks like everything necessary to render custom actions for vm details on the SUI is available. 💯

Copy link
Member

@abellotti abellotti left a comment

Choose a reason for hiding this comment

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

this is looking great @imtayadeway a couple of questions/comments. Thanks !!

}
expect(response.parsed_body).to include(expected)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

would you prefer to move these tests to custom_actions_spec.rb ? maybe some stuff can be DRYed up in future PRs between services and vms custom actions specs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might, but prefer to do that in a future PR. The current specs there are set up assuming that the service template is at the center of what's being tested, so would at least need to scope the existing tests in a new context. The resultant diff I thought would be unnecessarily large for this PR

Copy link
Member

Choose a reason for hiding this comment

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

👍


after(:create) do |custom_button_set, evaluator|
custom_button_set.replace_children(evaluator.children)
end
Copy link
Member

Choose a reason for hiding this comment

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

why was this needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it made these a bit nicer (rather than having to do the replace_children after each one).

Copy link
Member

Choose a reason for hiding this comment

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

ahh ok, and this does not affect current users of this Factory outside our API ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not, since currently setting :children in this way would result in:

     NoMethodError:
       undefined method `children=' for #<CustomButtonSet:0x0055c0df9d1ca0>
       Did you mean?  children
                      child_rels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abellotti I guess it could be possible to build a custom button set, add children, then save. I'll change this to non-destructively shovel children on instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abellotti updated in e3c785d

Copy link
Member

Choose a reason for hiding this comment

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

😍

@imtayadeway imtayadeway force-pushed the api/vm-custom-actions branch 2 times, most recently from 3a34604 to ab015e0 Compare April 19, 2017 22:12
CustomButton.buttons_for(self).select { |b| b.parent.nil? }
end

def generic_custom_buttons
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abellotti just updated with this to make the contract between this mixin and the host more explicit. It might be nice to add some lower-level tests for Vms now that this has been included - LMK if you'd rather I do that here or in a follow up. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

this is fine. Thanks.

@jjlangholtz
Copy link
Contributor

@imtayadeway I don't know if it is more suitable for this PR or a follow-up, but I am investigating a custom actions BZ and believe there is an error in #custom_action_buttons.

From the SUI, we are using #custom_actions to retrieve all buttons and button_groups, however when a custom action is invoked it uses #custom_action_buttons which does not include the generic buttons (buttons assigned to button_groups) leading to an exception.

@imtayadeway
Copy link
Contributor Author

@miq-bot add-label wip

@miq-bot miq-bot changed the title Enable custom actions for Vms API [WIP] Enable custom actions for Vms API Apr 21, 2017
@miq-bot miq-bot added the wip label Apr 21, 2017
@imtayadeway imtayadeway force-pushed the api/vm-custom-actions branch from 83e1b51 to e0daf3a Compare April 21, 2017 18:29
@imtayadeway
Copy link
Contributor Author

@abellotti marking this as WIP as it will need to be rebased onto #14845 (which I pulled out of this PR)

@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2017

This pull request is not mergeable. Please rebase and repush.

@imtayadeway imtayadeway force-pushed the api/vm-custom-actions branch from e0daf3a to 3c87bbe Compare April 26, 2017 18:32
@imtayadeway
Copy link
Contributor Author

@miq-bot rm-label wip

@miq-bot miq-bot changed the title [WIP] Enable custom actions for Vms API Enable custom actions for Vms API Apr 26, 2017

def generic_button_group
CustomButton.buttons_for("Service").select { |button| !button.parent.nil? }
end
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be defined here, just the one on line 48

@imtayadeway imtayadeway force-pushed the api/vm-custom-actions branch from 3c87bbe to 8318d5c Compare April 26, 2017 18:45
@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2017

Checked commits imtayadeway/manageiq@01d1771~...8318d5c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

@abellotti
Copy link
Member

Thanks @imtayadeway this is a great enhancement !! 🎵

@abellotti abellotti added this to the Sprint 60 Ending May 8, 2017 milestone Apr 26, 2017
@abellotti abellotti merged commit a1d985c into ManageIQ:master Apr 26, 2017
@chessbyte
Copy link
Member

@abellotti @gtanzillo do you feel comfortable labeling this fine/yes? If so, please do so.

@chrispy1
Copy link

@miq-bot add_label blocker

simaishi pushed a commit that referenced this pull request May 18, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 28fa95221971cf0baee0d806e12208d78bd1b6ba
Author: Alberto Bellotti <[email protected]>
Date:   Wed Apr 26 15:13:14 2017 -0400

    Merge pull request #14817 from imtayadeway/api/vm-custom-actions
    
    Enable custom actions for Vms API
    (cherry picked from commit a1d985ca1dd6be423c8623cef4a740cc6acef4e1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1450502

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

Successfully merging this pull request may close these issues.

8 participants