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

Fixes #24072 - freeip with compute resource via hostgroup #5745

Merged
merged 3 commits into from
Jul 3, 2018

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Jun 26, 2018

In #5790, we added an option to set the compute resource via hostgroup.
There was one thing missed though: the freeip not getting assigned
automagically, when the compute resource was selected this way.

After this change, we make sure to check for the ip addresses (and other
potential subnet related refreshes) after the hostgroup is selected, in
case the interfaces were populated during the hostgroup change.

@theforeman-bot
Copy link
Member

Issues: #24072

@iNecas
Copy link
Member Author

iNecas commented Jun 26, 2018

@xprazak2 since you worked on #5790, mind looking at this PR?

@xprazak2
Copy link
Contributor

It does take care of ips not being loaded, however the compute attributes for network are not loaded when hostgroup with compute resource and compute profile is changed, so users have to open the network modal anyway to change them.

compute profile selected selected:

hostgroup selected:

@@ -178,6 +180,46 @@ class HostJSTest < IntegrationTestWithJavascript
assert_equal overridden_hostgroup.environment.name, environment
end

test 'choosing a hostgroup with compute resource works' do
hostgroup = FactoryBot.create(:hostgroup, :with_environment, :with_subnet, :with_domain, :with_compute_resource)
hostgroup.subnet.update_attributes!(ipam: IPAM::MODES[:db])

Choose a reason for hiding this comment

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

Rails/ActiveRecordAliases: Use update! instead of update_attributes!.

@iNecas
Copy link
Member Author

iNecas commented Jun 29, 2018

Thanks for pointing that out: it turned out there was more issues with the 'deploy on': I've extended the Redmine issue to mention the others as well (seems wasteful to create separate issues for those, as they all together mean the whole feature is not very usable).

I've added the fixes in separate commits, and I would prefer not squashing those, as it makes it easier to see what changes belong together, as opposed to one big commit.

I've also encouraged myself to write an integration tests for this. I've learned about the poltergeist more than I wished, but it helped finding some other issues as well and hopefully, it will help with regressions in this area in the future.

@xprazak2
Copy link
Contributor

xprazak2 commented Jul 2, 2018

[test foreman]

@xprazak2
Copy link
Contributor

xprazak2 commented Jul 2, 2018

Working nicely now, APJ.

Without this fix, the 'Virtual Machine' tab was giving:

   'mybox' not found on 'libvirt (Libvirt)' 'mybox' could
   be deleted or 'libvirt (Libvirt)' is not responding.
When compute resource is set via compute profile, we're not sending it
via the form (as it's disabled). Therefore, we need to search for in via
hostgroup.
In theforeman#5790, we added an option to set the compute resource via hostgroup.
There was one thing missed though: the freeip not getting assigned
automagically, when the compute resource was selected this way.

After this change, we make sure to check for the ip addresses (and other
potential subnet related refreshes) after the hostgroup is selected, in
case the interfaces were populated during the hostgroup change.
self.compute_attributes = compute_resource.compute_profile_attributes_for(compute_profile_id)
if compute_profile_present?
self.compute_attributes = compute_resource.compute_profile_attributes_for(compute_profile_id)
elsif compute_resource
Copy link
Member Author

Choose a reason for hiding this comment

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

Added check on compute resource presence when setting the compute attributes (with pure else, the bare-metal test was failing)

Copy link
Contributor

@xprazak2 xprazak2 left a comment

Choose a reason for hiding this comment

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

👍

@iNecas
Copy link
Member Author

iNecas commented Jul 2, 2018

@ares mind having a look as someone with merge rights (if you know what I mean ;-)

@xprazak2 xprazak2 merged commit dc31392 into theforeman:develop Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants