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 #15717 - Unable to assign a VM in Site to Cluster without Site #15763

Merged
merged 5 commits into from
Jun 20, 2024
Merged

Fixes #15717 - Unable to assign a VM in Site to Cluster without Site #15763

merged 5 commits into from
Jun 20, 2024

Conversation

nopg
Copy link
Contributor

@nopg nopg commented Apr 18, 2024

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.

Copy link
Contributor

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.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label May 21, 2024
@arthanson
Copy link
Collaborator

@nopg can you please fix the merge conflicts

@nopg
Copy link
Contributor Author

nopg commented May 23, 2024

Done.

@jeremystretch jeremystretch requested a review from arthanson May 30, 2024 13:05
Copy link
Collaborator

@arthanson arthanson left a 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.
Monosnap Editing virtual machine child vm | NetBox 2024-05-30 12-04-54

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.

@nopg
Copy link
Contributor Author

nopg commented May 31, 2024

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.

@jeremystretch jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Jun 4, 2024
@nopg
Copy link
Contributor Author

nopg commented Jun 4, 2024

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 nopg requested a review from arthanson June 5, 2024 16:22
@arthanson
Copy link
Collaborator

@nopg The dropdown works now but there is a validation error if you try to save
Monosnap Editing virtual machine vm9 | NetBox 2024-06-07 08-37-46

@nopg
Copy link
Contributor Author

nopg commented Jun 7, 2024

@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.

vms

@arthanson arthanson self-assigned this Jun 10, 2024
@arthanson
Copy link
Collaborator

@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).
Monosnap Editing virtual machine vm9 | NetBox 2024-06-11 11-21-45

@arthanson arthanson removed their request for review June 11, 2024 18:25
@arthanson arthanson removed their assignment Jun 11, 2024
@nopg
Copy link
Contributor Author

nopg commented Jun 12, 2024

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.

@nopg nopg requested a review from arthanson June 18, 2024 19:30
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Thanks @nopg!

@jeremystretch jeremystretch merged commit 582ede8 into netbox-community:develop Jun 20, 2024
3 checks passed
@nopg nopg deleted the 15717-vm-cluster-site branch July 8, 2024 16:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to assign VM/Device in one site to cluster that has no site
3 participants