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 #37714 - inherit overrides deploy on in hostgroup #10265

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Aug 7, 2024

if the hostgroup parent has a compute resource, the child hostgroup cant be set to have "bare metal", since empty value ("bare metal") = inherit.
In this pr I managed to make it work similarly to the host page, which is not perfect.
I added on top of submit_with_all_params to just reuse existing logic and not building new one.

@github-actions github-actions bot added the Legacy JS PRs making changes in the legacy Javascript stack label Aug 7, 2024
:id,
:to_label,
{
:disable_button_enabled => hostgroup_inherited_by_default?(:compute_resource_id, @hostgroup),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to hide this for parent hostgroups?

Copy link
Member Author

Choose a reason for hiding this comment

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

No since they can also be children

Copy link
Contributor

Choose a reason for hiding this comment

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

What about for top-level host groups which don't have a parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could hide it, but it isn't hidden in hosts if they have no host group

@MariaAga
Copy link
Member Author

I added on top of submit_with_all_params to just reuse existing logic and not building new one.

That didnt work, so I added an event listener

@sjha4
Copy link
Contributor

sjha4 commented Aug 14, 2024

The child hostgroup doesn't seem to be able to inherit the parent hostgroup. The compute resource gets set to empty/bare metal even when the form is set to inherit and populated with parent's compute resource.

@MariaAga
Copy link
Member Author

MariaAga commented Aug 16, 2024

Changed the pr so now on submit hostgroup makes sure that the compute resource is included.
Also changed the inherit button behaviour for compute resource in hosts, when a user selects a host group the code saves the host groups compute resource, and when "inherit" is clicked it disables the select and selects the host groups compute resource. (in host & hostgroup forms)
before this pr in hosts if the compute resource was inherited it didnt always check the fields before the submit, so for example it didnt check libvirt connection if it was inherited, it does now

@sjha4
Copy link
Contributor

sjha4 commented Aug 19, 2024

Works now for setting compute resource id in the form. The inherit button is a bit confusing as when you click inherit on a child group, it populates the form as Bare Metal and disables the form which is not the inherited value..

@MariaAga
Copy link
Member Author

MariaAga commented Aug 20, 2024

Thanks! added a fix that sets the inherit value on load, and not only on hostgroup change

@sjha4
Copy link
Contributor

sjha4 commented Aug 20, 2024

I have a parent HG for example with compute resource as libvirt-test:
Screenshot from 2024-08-20 13-43-08

On a child HG, the inherit button sets the compute resource as bare metal (nil) and then clicking the button switches enabled/disabled state of the input field. It's more logical to populate the field with parent's compute resource when the button is clicked vs setting it to bare metal?

Screenshot from 2024-08-20 13-43-53
Screenshot from 2024-08-20 13-44-10

@MariaAga
Copy link
Member Author

Clicking on inherit should populate the deploy on with a the parent value,
screencap of parent libvirt, and parent bare metal
https://github.com/user-attachments/assets/b64398ab-6d6d-4661-a349-830a04da0f44

@MariaAga
Copy link
Member Author

removed an unused function

@sjha4
Copy link
Contributor

sjha4 commented Aug 22, 2024

Here's what I am seeing which is a little confusing:
When I click the nest button and land on a create HG page, it loads up with the correct compute resource selected and the field disabled.
Nest-load

If I click on "Inherit", the 1st click enables the Compute resource dropdown.
Inherit-1stclick

If I click "Inherit" now, it sets compute resource to Bare metal and disables the dropdown.
Clicking inherit after this enables/disables the dropdown with Bare metal.
Inherit-3rdClick

I'd expect the field to change to the actual inherited value: "test-compute-resource" whenever inherit button is clicked.

@kmalyjur
Copy link
Contributor

If I click "Inherit" now, it sets compute resource to Bare metal and disables the dropdown.

It works fine for me - after clicking the button for the second time the compute resource stays there.

@MariaAga
Copy link
Member Author

Maybe emptying cache will resolve it? @sjha4

@sjha4
Copy link
Contributor

sjha4 commented Aug 28, 2024

I cleared my cache and opened a new incognito and this is what I see upon 2 clicks on inherit button on a nested create form.
Screencast from 2024-08-28 16-23-42.webm

@MariaAga
Copy link
Member Author

I managed to reproduce this error, but then I needed to restart the server (for something else) and the error was gone, so maybe thats the issue? sorry for this reproduction hunt @sjha4

@MariaAga
Copy link
Member Author

MariaAga commented Sep 2, 2024

Rebase fixed the failing tests

Copy link
Contributor

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Ack 🎉

Copy link
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

ACK based on @sjha4 👍

@jeremylenz jeremylenz merged commit 8e10194 into theforeman:develop Sep 6, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy JS PRs making changes in the legacy Javascript stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants