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

Allow move_into_folder to optionally take string arg for automate engine exposure #19086

Merged
merged 1 commit into from
Aug 12, 2019
Merged

Allow move_into_folder to optionally take string arg for automate engine exposure #19086

merged 1 commit into from
Aug 12, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Aug 1, 2019 •
edited
Loading

We have to queue ems_folders for the automate exposure of move_into_folder, so this method needs a change to handle being passed a string.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1716858
@miq-bot add_label enhancement

Related:

ManageIQ/manageiq-automation_engine#344

@d-m-u d-m-u mentioned this pull request Aug 1, 2019
Merged
@d-m-u d-m-u changed the title [wip] Allow folder to optionally be ems_ref for automate engine exposure [wip] Allow move_into_folder to optionally take string arg for automate engine exposure Aug 1, 2019
@miq-bot miq-bot added the wip label Aug 1, 2019
@d-m-u d-m-u changed the title [wip] Allow move_into_folder to optionally take string arg for automate engine exposure Allow move_into_folder to optionally take string arg for automate engine exposure Aug 1, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 1, 2019

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Aug 1, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 1, 2019

@miq-bot assign @tinaafitz

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 1, 2019

@miq-bot add_label ivanchuk/yes

@gmcculloug gmcculloug requested a review from agrare August 1, 2019 16:00
@agrare
Copy link
Member

agrare commented Aug 1, 2019

@d-m-u maybe something like def move_into_folder(folder_or_id) then check if it is already .kind_of?(EmsFolder) ?

@agrare
Copy link
Member

agrare commented Aug 1, 2019

Also a little concerned, as every one of these methods expect an instance of the object so this will be inconsistent. Should we be doing this to every operation?

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 2, 2019 •
edited
Loading

@agrare I wondered about that. with an instance, it hits the check on https://github.com/ManageIQ/manageiq/pull/8963/files#diff-d8c450b508868941c2b0c5884e718324R121

@agrare
Copy link
Member

agrare commented Aug 2, 2019

@d-m-u yeah none of these methods were intended to be called over the queue, following the normal pattern we'd have e.g. move_into_folder_queue(host_id) which would lookup the host and call move_into_folder(host). We could add _queue methods for the other methods here as well.

@d-m-u d-m-u force-pushed the allow_move_into_folder_to_take_ems_ref branch 3 times, most recently from 3d2ba61 to 9e1683c Compare August 2, 2019 14:31
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 5, 2019

@agrare can I address #19086 (comment) as part of later work so I can close the ticket, you okay with that?

@agrare
Copy link
Member

agrare commented Aug 5, 2019

Yeah if course

@gmcculloug gmcculloug assigned gmcculloug and unassigned tinaafitz Aug 5, 2019
@gmcculloug
Copy link
Member

Discussed with @d-m-u and we are going to looking into doing the queue work on this PR. 🌮

@d-m-u d-m-u force-pushed the allow_move_into_folder_to_take_ems_ref branch from 9e1683c to a7006be Compare August 7, 2019 15:16
@d-m-u d-m-u force-pushed the allow_move_into_folder_to_take_ems_ref branch from a7006be to 9ca52d8 Compare August 7, 2019 17:14
agrare
agrare approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

gmcculloug
gmcculloug reviewed Aug 8, 2019
View reviewed changes
Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

Looks good, just missing tests.

@d-m-u d-m-u force-pushed the allow_move_into_folder_to_take_ems_ref branch from 9ca52d8 to 6427c2a Compare August 12, 2019 14:22
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 12, 2019

@lfu please review

@d-m-u d-m-u force-pushed the allow_move_into_folder_to_take_ems_ref branch 3 times, most recently from 34db7eb to d0eeb27 Compare August 12, 2019 14:51
@d-m-u d-m-u force-pushed the allow_move_into_folder_to_take_ems_ref branch from d0eeb27 to f1a720b Compare August 12, 2019 14:52
@miq-bot
Copy link
Member

miq-bot commented Aug 12, 2019

Checked commit d-m-u@f1a720b with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug gmcculloug requested a review from lfu August 12, 2019 15:10
@gmcculloug gmcculloug merged commit e088fad into ManageIQ:master Aug 12, 2019
@gmcculloug gmcculloug added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 12, 2019
simaishi pushed a commit that referenced this pull request Aug 12, 2019
@gmcculloug @simaishi
Merge pull request #19086 from d-m-u/allow_move_into_folder_to_take_e…
95cbe8e 
…ms_ref

Allow move_into_folder to optionally take string arg for automate engine exposure

(cherry picked from commit e088fad)

https://bugzilla.redhat.com/show_bug.cgi?id=1716858
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 95cbe8eba21b5f2c55d08a61335118ee72f933ae
Author: Greg McCullough <[email protected]>
Date:   Mon Aug 12 11:30:35 2019 -0400

    Merge pull request #19086 from d-m-u/allow_move_into_folder_to_take_ems_ref
    
    Allow move_into_folder to optionally take string arg for automate engine exposure
    
    (cherry picked from commit e088fad7b7349ec960588498dc5d854b12486845)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1716858

@d-m-u d-m-u deleted the allow_move_into_folder_to_take_ems_ref branch September 26, 2019 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

@agrare agrare agrare approved these changes

@lfu lfu lfu approved these changes

Assignees

@gmcculloug gmcculloug

Labels
changelog/yes enhancement ivanchuk/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@d-m-u @agrare @gmcculloug @miq-bot @simaishi @lfu @tinaafitz