Skip to content

Conversation

@RobertH1993
Copy link

Closes: #18658

Adds start on boot field to VirtualMachine model

@RobertH1993
Copy link
Author

For reviewers

I had doubts about how I would do the displaying of the start on boot status. The 'status' field in the model VirtualMachine uses a badge. I decided to not use a badge but plaintext or a placeholder when the 'start_on_boot' field is empty. This was a personal choice but can be changed ofc. I did specify colors in the choices model for when a badge is preferred.

I took inspiration from another merge request that added a status field to some model, so I guess I got everything covered, bulk_imports, csv_imports, etc. I added this to the tests too and they run succesfully.

@jnovinger jnovinger requested review from a team and arthanson and removed request for a team November 6, 2025 15:30
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.

Good work, just some cleanup needed mostly on the color and merge the migrations together.

@@ -0,0 +1,18 @@
# Generated by Django 5.2.7 on 2025-11-05 13:45
Copy link
Collaborator

Choose a reason for hiding this comment

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

These migrations should be collapsed together

Copy link
Author

@RobertH1993 RobertH1993 Nov 8, 2025

Choose a reason for hiding this comment

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

I removed this migration, like you said in another feedback item there isn't really a case where the field should be empty. There is always some kind of behaviour when the hypervisor reboots

The start on boot setting from the hypervisor.

!!! tip
Additional status may be defined by setting `VirtualMachine.start_on_boot` under the [`FIELD_CHOICES`](../../configuration/data-validation.md#field_choices) configuration parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Additional statuses

STATUS_LAST_STATE = 'laststate'

CHOICES = [
(STATUS_ON, _('On'), 'green'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are defining color here but it isn't used, it is also inconsistent - in the list view it is always shown as a grey badge no matter the status, in the detail view it is shown as text. If there is a color/badge then it should be shown as such in both places.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed this by adding a get_start_on_boot_color method to VirtualMachine model.

default=VirtualMachineStatusChoices.STATUS_ACTIVE,
verbose_name=_('status')
)
start_on_boot = models.CharField(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is assigning a default and then allowing blank=True and null=True you probably don't want the blank and null=True there, check the status field for an example. Is there a reason to have an unset state?

@RobertH1993 RobertH1993 requested a review from arthanson November 8, 2025 09:43
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.

2 participants