Skip to content

Add Puppet environment bulk actions - #443

Open
nadjaheitmann wants to merge 1 commit into
masterfrom
environment_bulk_action
Open

Add Puppet environment bulk actions#443
nadjaheitmann wants to merge 1 commit into
masterfrom
environment_bulk_action

Conversation

@nadjaheitmann

Copy link
Copy Markdown
Collaborator

Generated-by: OpenAI Codex codex@openai.com

@nadjaheitmann
nadjaheitmann marked this pull request as draft May 20, 2026 07:50
@nadjaheitmann
nadjaheitmann force-pushed the environment_bulk_action branch 6 times, most recently from 7b47350 to b3a99a0 Compare May 21, 2026 10:37
@nadjaheitmann
nadjaheitmann force-pushed the environment_bulk_action branch 8 times, most recently from 3c788d1 to 6e4a70a Compare July 17, 2026 10:08
Co-Authored-By: OpenAI Codex <codex@openai.com>
@nadjaheitmann
nadjaheitmann force-pushed the environment_bulk_action branch from 6e4a70a to a6a902f Compare July 17, 2026 10:29
@nadjaheitmann
nadjaheitmann marked this pull request as ready for review July 17, 2026 12:00

const handleError = response => {
handleModalClose();
dispatch(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread config/api_routes.rb
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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 /api/foreman_puppet/api/v2/hosts/bulk/change_environment, while the change_puppet_proxy host bulk-action's path is /api/hosts/bulk/change_puppet_proxy.
The latter seems more correct to me.

def get_environment_for(host, id)
if id == 'inherit' && host.hostgroup.present?
host.hostgroup.environment
host.hostgroup.puppet&.environment

Copy link
Copy Markdown
Contributor

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?

Comment on lines +65 to 67
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'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These might need changing based on the result in the above comment.


def put_change_environment(params:, session: nil)
original_routes = @routes
@routes = ForemanPuppet::Engine.routes

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This would also be obsolete, when api_routes is changed.

Comment on lines +20 to +29
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The test setup should not already fulfill the test-assertion.

Suggested change
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)
inherited_environment = FactoryBot.create(:environment, organizations: [organization], locations: [location])
host_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: host_environment,
hostgroup: hostgroup,
organization: organization,
location: location)
assert_not_equal host_environment, inherited_environment

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants