-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 #15717 - Unable to assign a VM in Site to Cluster without Site #15763
Fixes #15717 - Unable to assign a VM in Site to Cluster without Site #15763
Conversation
This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken. |
@nopg can you please fix the merge conflicts |
Done. |
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.
@nopg I tried this PR -
I go to edit a VM that has a site assigned and I can't get the unassigned cluster in the dropdown (see screenshot below) I have created a 'tcluster' that doesn't appear in the selection. I think the queryset for the dropdown needs to be updated.
Note: without the PR I can assign a cluster (not assigned to a site) to a device that is assigned to a device fine, so I think this only applies to VM.
I swear i checked that myself and it was working (at least back on 3.x)! But yes I see that issue as well. I've got a lot going on the next few days but plan on updating this as soon as I can, just wanted to reply here first. |
Ok @arthanson, done. As you mentioned this doesn't apply to Devices just VM's. I removed the query_params from VM's/Cluster, which is how it looks on the Device side as well. |
@nopg The dropdown works now but there is a validation error if you try to save |
@arthanson I can't reproduce that...I can save or create a VM-with-site into my Cluster without a site now, screenshot below. The validation should have been part of the initial change I made to virtualmachine.py in this PR. |
@nopg I see the issue, I actually had TC assigned to a different site, the problem is that the dropdown is now displaying Clusters in all sites so you can select an invalid cluster and get the error message above. I have 3 clusters to test: TC is unassigned, TC2 is a different site then the VM and TC3 is the same site. Only TC and TC3 should be showing in the dropdown (see screenshot). |
Ok @arthanson, sounds good. I had initially noticed the behavior you are describing, but saw that this same behavior also already existed with Devices, so decided to just 'match' the Devices behavior and leave it be. After looking into it though, I was able to produce the desired behavior (showing only same-site Clusters in the list, or Clusters not attached to any Site) with query_params, so I also updated the associated cluster query_params for Device as well as VirtualMachine. I assume at this point it's ok to include that in this PR. We are hopefully good to go at this point. |
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.
Thanks @nopg!
Fixes: #15717
Adds to existing IF statement for VirtualMachine.clean() (to match existing check for Device.clean()) to allow for VM to be assigned to a Cluster without a Site.
Updated existing VM test to check for this new use case, and added Device tests for Clusters with Sites matching/not matching as well.