-
Notifications
You must be signed in to change notification settings - Fork 29
Add Puppet environment bulk actions #443
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,8 @@ | |
| ForemanPuppet::Engine.routes.draw do | ||
| namespace :api, defaults: { format: 'json' } do | ||
| scope '(:apiv)', module: :v2, defaults: { apiv: 'v2' }, apiv: /v1|v2/, constraints: ApiConstraints.new(version: 2, default: true) do | ||
| match 'hosts/bulk/change_environment', to: 'hosts_bulk_actions#change_environment', via: [:put] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't it be better to move this route to the bulk action area in line 5? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, at the moment the path of the endpoint is evaluated to |
||
|
|
||
| constraints(id: %r{[^/]+}) do | ||
| resources :config_groups, except: %i[new edit] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ | |
| p.actions << 'hosts/update_multiple_environment' | ||
| p.actions << 'hosts/select_multiple_puppet_proxy' | ||
| p.actions << 'hosts/update_multiple_puppet_proxy' | ||
| p.actions << 'foreman_puppet/api/v2/hosts_bulk_actions/change_environment' | ||
| p.actions << 'foreman_puppet/api/v2/hosts_bulk_actions/change_puppet_proxy' | ||
| p.actions << 'foreman_puppet/api/v2/hosts_bulk_actions/remove_puppet_proxy' | ||
|
Comment on lines
+65
to
67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These might need changing based on the result in the above comment. |
||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,8 +16,105 @@ class HostsBulkActionsControllerTest < ActionController::TestCase | |
| organization: host.organization, | ||
| location: host.location) | ||
| end | ||
| let(:environment) { FactoryBot.create(:environment, organizations: [host.organization], locations: [host.location]) } | ||
| let(:hostgroup_environment) { FactoryBot.create(:environment, organizations: [host.organization], locations: [host.location]) } | ||
| let(:hostgroup) do | ||
| FactoryBot.create(:hostgroup, :with_puppet_enc, | ||
| environment: hostgroup_environment, | ||
| organizations: [host.organization], | ||
| locations: [host.location]) | ||
| end | ||
| let(:proxy) { FactoryBot.create(:puppet_and_ca_smart_proxy, organizations: [host.organization], locations: [host.location]) } | ||
|
|
||
| def put_change_environment(params:, session: nil) | ||
| original_routes = @routes | ||
| @routes = ForemanPuppet::Engine.routes | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would also be obsolete, when api_routes is changed. |
||
| put :change_environment, params: params, session: session | ||
| ensure | ||
| @routes = original_routes | ||
| end | ||
|
|
||
| test 'changes puppet environment for selected hosts' do | ||
| put_change_environment(params: bulk_params.merge(environment_id: environment.id)) | ||
|
|
||
| assert_response :success | ||
| assert_equal environment.id, host2.reload.puppet.environment_id | ||
| assert_equal environment.id, host.reload.puppet.environment_id | ||
| end | ||
|
|
||
| test 'inherits puppet environment from hostgroups for selected hosts' do | ||
| host.update!(hostgroup: hostgroup) | ||
| host2.update!(hostgroup: hostgroup) | ||
|
|
||
| put_change_environment(params: bulk_params.merge(environment_id: 'inherit')) | ||
|
|
||
| assert_response :success | ||
| assert_equal hostgroup_environment.id, host2.reload.puppet.environment_id | ||
| assert_equal hostgroup_environment.id, host.reload.puppet.environment_id | ||
| end | ||
|
|
||
| test 'clears puppet environment for selected hosts' do | ||
| host.puppet.update!(environment: environment) | ||
| host2.puppet.update!(environment: environment) | ||
|
|
||
| put_change_environment( | ||
| params: bulk_params.merge(environment_id: nil), | ||
| session: set_session_user | ||
| ) | ||
|
|
||
| assert_response :success | ||
| assert_nil host.reload.puppet.environment | ||
| assert_nil host2.reload.puppet.environment | ||
| end | ||
|
|
||
| test 'returns error when puppet environment is missing' do | ||
| missing_environment_id = 999_999 | ||
|
|
||
| put_change_environment( | ||
| params: bulk_params.merge(environment_id: missing_environment_id), | ||
| session: set_session_user | ||
| ) | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = JSON.parse(@response.body) | ||
| assert_equal "A Puppet environment with id #{missing_environment_id} could not be found.", | ||
| response.dig('error', 'message') | ||
| end | ||
|
|
||
| test 'returns error when changing puppet environment fails for some hosts' do | ||
| ::BulkHostsManager.any_instance.expects(:change_puppet_environment) | ||
| .with(environment) | ||
| .returns([host2.id]) | ||
|
|
||
| put_change_environment( | ||
| params: bulk_params.merge(environment_id: environment.id), | ||
| session: set_session_user | ||
| ) | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = JSON.parse(@response.body) | ||
| assert_equal 'Failed to change environment for 1 host', | ||
| response.dig('error', 'message') | ||
| assert_equal [host2.id], response.dig('error', 'failed_host_ids') | ||
| end | ||
|
|
||
| test 'returns error when puppet environment is outside host taxonomies' do | ||
| invalid_environment = FactoryBot.create(:environment, | ||
| organizations: [host.organization], | ||
| locations: [FactoryBot.create(:location)]) | ||
|
|
||
| put_change_environment( | ||
| params: bulk_params.merge(environment_id: invalid_environment.id), | ||
| session: set_session_user | ||
| ) | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = JSON.parse(@response.body) | ||
| assert_equal 'Selected Puppet environment is not assigned to the proper organization and/or location for all hosts.', | ||
| response.dig('error', 'message') | ||
| assert_equal [host.id, host2.id].sort, response.dig('error', 'failed_host_ids').sort | ||
| end | ||
|
|
||
| test 'changes puppet proxy for selected hosts' do | ||
| put :change_puppet_proxy, | ||
| params: bulk_params.merge(proxy_id: proxy.id, ca_proxy: false) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,46 @@ class BulkHostsManagerTest < ActiveSupport::TestCase | |||||||||||||||||||||||||||||||||||||||||||||||
| let(:hosts) { FactoryBot.create_list(:host, 2, :with_puppet_enc) } | ||||||||||||||||||||||||||||||||||||||||||||||||
| let(:manager) { ::BulkHostsManager.new(hosts: hosts) } | ||||||||||||||||||||||||||||||||||||||||||||||||
| let(:proxy) { FactoryBot.create(:puppet_smart_proxy) } | ||||||||||||||||||||||||||||||||||||||||||||||||
| let(:environment) { FactoryBot.create(:environment, organizations: [hosts.first.organization], locations: [hosts.first.location]) } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| test 'changes puppet environment for hosts' do | ||||||||||||||||||||||||||||||||||||||||||||||||
| manager.change_puppet_environment(environment) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| hosts.each do |host| | ||||||||||||||||||||||||||||||||||||||||||||||||
| assert_equal environment.id, host.reload.puppet.environment_id | ||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| test 'inherits puppet environment from hostgroup' do | ||||||||||||||||||||||||||||||||||||||||||||||||
| organization = FactoryBot.create(:organization) | ||||||||||||||||||||||||||||||||||||||||||||||||
| location = FactoryBot.create(:location) | ||||||||||||||||||||||||||||||||||||||||||||||||
| inherited_environment = FactoryBot.create(:environment, organizations: [organization], locations: [location]) | ||||||||||||||||||||||||||||||||||||||||||||||||
| hostgroup = FactoryBot.create(:hostgroup, :with_puppet_enc, | ||||||||||||||||||||||||||||||||||||||||||||||||
| environment: inherited_environment, | ||||||||||||||||||||||||||||||||||||||||||||||||
| organizations: [organization], | ||||||||||||||||||||||||||||||||||||||||||||||||
| locations: [location]) | ||||||||||||||||||||||||||||||||||||||||||||||||
| inherited_hosts = FactoryBot.create_list(:host, 2, :with_puppet_enc, | ||||||||||||||||||||||||||||||||||||||||||||||||
| environment: inherited_environment, | ||||||||||||||||||||||||||||||||||||||||||||||||
| hostgroup: hostgroup, | ||||||||||||||||||||||||||||||||||||||||||||||||
| organization: organization, | ||||||||||||||||||||||||||||||||||||||||||||||||
| location: location) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test setup should not already fulfill the test-assertion.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ::BulkHostsManager.new(hosts: inherited_hosts).change_puppet_environment('inherit') | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| inherited_hosts.each do |host| | ||||||||||||||||||||||||||||||||||||||||||||||||
| assert_equal inherited_environment.id, host.reload.puppet.environment_id | ||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| test 'clears puppet environment when environment is nil' do | ||||||||||||||||||||||||||||||||||||||||||||||||
| hosts.each { |host| host.puppet.update!(environment: environment) } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| manager.change_puppet_environment(nil) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| hosts.each do |host| | ||||||||||||||||||||||||||||||||||||||||||||||||
| assert_nil host.reload.puppet.environment | ||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| test 'changes puppet proxy for hosts' do | ||||||||||||||||||||||||||||||||||||||||||||||||
| manager.change_puppet_proxy(proxy, false) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
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.
I guess, this is a issues which is solved within this PR?