-
Notifications
You must be signed in to change notification settings - Fork 287
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
Fixes #24429 - Module stream host bulk actions #7736
Fixes #24429 - Module stream host bulk actions #7736
Conversation
Issues: #24429 |
476cfe0
to
161e45d
Compare
{ action: 'install', description: translate("Install")}, | ||
{ action: 'update', description: translate("Update")}, | ||
{ action: 'remove', description: translate("Remove")} | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have an angular service for DRYing this up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, was planning on that
5c53f8d
to
6ec0540
Compare
eacb6f5
to
802dcb1
Compare
f57d911
to
dbbe532
Compare
app/views/katello/api/v2/hosts_bulk_actions/module_streams.json.rabl
Outdated
Show resolved
Hide resolved
[test katello] |
1d7ff12
to
4337eb4
Compare
4337eb4
to
f784c98
Compare
Code changes LGTM. I was originally thinking if some of the stuff in views/content-host-bulk-module-streams-modal.html could be DRY'ed up, but I am hesitant to suggest that now considering content hosts module streams page will change once we get information from subman on enabled repositories. |
[test katello] |
options = {} | ||
options[:group] = [:name, :stream] | ||
options[:resource_class] = Katello::ModuleStream | ||
host_module_streams = Katello::ModuleStream.available_for_hosts(@hosts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does @hosts get set here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh find_editable_hosts is set to :except =>
nevermind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved from a code perspective. haven't' tested though
end | ||
find_bulk_hosts('edit_hosts', | ||
included: { | ||
ids: params[:host_ids], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a bug. It expects host_ids to be an array like :host_ids =>[1,2,3]
, while what gets sent to remote command controller is something like :host_ids =>"1,2,3"
so need to change this to params[:host_ids].try(:split, ',')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be do something like params[:host_ids].is_a?(Array) ? params[:host_ids]: params[:host_ids].try(:split, ',')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlsherrill would you think something like this might be better in Concerns::Api::V2::BulkHostsExtensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, the browser should be sending an array IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm too many places to fix that then imo.
Ah, if this is what the rex actions are doing, then I guess I'm okay with
splitting it in the rex controller
…On Wed, Oct 10, 2018, 5:07 PM Partha Aji ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/controllers/katello/remote_execution_controller.rb
<#7736 (comment)>:
> @@ -28,11 +30,13 @@ def prepare_composer
end
def hosts
- if params[:scoped_search].present?
- params[:scoped_search]
- else
- ::Host.where(:id => params[:host_ids].try(:split, ','))
- end
+ find_bulk_hosts('edit_hosts',
+ included: {
+ ids: params[:host_ids],
hmm too many places to fix that then imo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7736 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYHRVPOIZHa3-4t0vSn0BkDFRPGqRTJks5ujmGlgaJpZM4XKl8z>
.
|
@johnpmitsch Any need of this Remove Button? |
@parthaa @omkarkhatavkar @jlsherrill all issues have been addressed in a new commit. I'm currently setting up my environment to test multiple content hosts |
@johnpmitsch looks good. wfm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
@johnpmitsch: Work Nicely Test Scenarios:
|
https://projects.theforeman.org/issues/25258 raised for running module stream actions on each host collections not all hosts. |
1 similar comment
https://projects.theforeman.org/issues/25258 raised for running module stream actions on each host collections not all hosts. |
This adds both bulk content host and host collection actions to manage module streams