-
Notifications
You must be signed in to change notification settings - Fork 20
Cmldev 531 implement pcl support for lab autostart feature #172
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
base: dev
Are you sure you want to change the base?
Cmldev 531 implement pcl support for lab autostart feature #172
Conversation
virl2_client/virl2_client.py
Outdated
| ) -> Lab: | ||
| """ | ||
| Create a new lab with optional title, description, and notes. | ||
| Create a new lab with optional title, description, notes, and autostart configuration. |
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.
You should not be able to create new lab and set autostart at the same time. You need to create lab and after that (if you are admin), you can setup autostart.
So, when lab is created, you cannot set values, so all the code below could be removed
marpauli-cisco
left a comment
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.
autostart_config changed to autostart
Can not set autostart parameters during lab create, therefore you can remove it along with tests from lab creation
virl2_client/models/lab.py
Outdated
| self._title = title | ||
| self._description = "" | ||
| self._notes = "" | ||
| self._autostart_config = { |
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.
| self._autostart_config = { | |
| self._autostart = { |
virl2_client/models/lab.py
Outdated
| def autostart_enabled(self) -> bool: | ||
| """Return whether autostart is enabled for the lab.""" | ||
| self.sync_topology_if_outdated() | ||
| return self._autostart_config["enabled"] |
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.
| return self._autostart_config["enabled"] | |
| return self._autostart["enabled"] |
virl2_client/models/lab.py
Outdated
| def autostart_priority(self) -> int | None: | ||
| """Return the autostart priority of the lab.""" | ||
| self.sync_topology_if_outdated() | ||
| return self._autostart_config["priority"] |
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.
| return self._autostart_config["priority"] | |
| return self._autostart["priority"] |
virl2_client/models/lab.py
Outdated
| def autostart_delay(self) -> int | None: | ||
| """Return the autostart delay of the lab.""" | ||
| self.sync_topology_if_outdated() | ||
| return self._autostart_config["delay"] |
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.
| return self._autostart_config["delay"] | |
| return self._autostart["delay"] |
virl2_client/models/lab.py
Outdated
| self._autostart_config = lab_dict.get( | ||
| "autostart_config", |
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.
| self._autostart_config = lab_dict.get( | |
| "autostart_config", | |
| self._autostart = lab_dict.get( | |
| "autostart", |
tests/test_client_library.py
Outdated
| ("autostart_delay", -1, "between 0 and 84600"), | ||
| ], | ||
| ) | ||
| def test_create_lab_autostart_validation( |
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.
as mentioned elsewhere, this test can be removed, because you cannot create lab and set autostart at the same time. As a metter of fact, I think, all tests in this file can be removed
tests/test_client_library_labs.py
Outdated
| lab.set_autostart_config(enabled=True, priority=500, delay=120) | ||
|
|
||
| assert lab._autostart_config == { | ||
| "enabled": True, | ||
| "priority": 500, | ||
| "delay": 120, | ||
| } | ||
| assert lab.autostart_enabled is True | ||
| assert lab.autostart_priority == 500 | ||
| assert lab.autostart_delay == 120 | ||
|
|
||
| # Test validation in convenience method | ||
| with pytest.raises( | ||
| ValueError, match="autostart_priority must be between 0 and 10000" | ||
| ): | ||
| lab.set_autostart_config(enabled=True, priority=15000) | ||
|
|
||
| with pytest.raises( | ||
| ValueError, match="autostart_delay must be between 0 and 84600" | ||
| ): | ||
| lab.set_autostart_config(enabled=True, delay=100000) |
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.
| lab.set_autostart_config(enabled=True, priority=500, delay=120) | |
| assert lab._autostart_config == { | |
| "enabled": True, | |
| "priority": 500, | |
| "delay": 120, | |
| } | |
| assert lab.autostart_enabled is True | |
| assert lab.autostart_priority == 500 | |
| assert lab.autostart_delay == 120 | |
| # Test validation in convenience method | |
| with pytest.raises( | |
| ValueError, match="autostart_priority must be between 0 and 10000" | |
| ): | |
| lab.set_autostart_config(enabled=True, priority=15000) | |
| with pytest.raises( | |
| ValueError, match="autostart_delay must be between 0 and 84600" | |
| ): | |
| lab.set_autostart_config(enabled=True, delay=100000) | |
| lab.set_autostart(enabled=True, priority=500, delay=120) | |
| assert lab._autostart == { | |
| "enabled": True, | |
| "priority": 500, | |
| "delay": 120, | |
| } | |
| assert lab.autostart_enabled is True | |
| assert lab.autostart_priority == 500 | |
| assert lab.autostart_delay == 120 | |
| # Test validation in convenience method | |
| with pytest.raises( | |
| ValueError, match="autostart_priority must be between 0 and 10000" | |
| ): | |
| lab.set_autostart(enabled=True, priority=15000) | |
| with pytest.raises( | |
| ValueError, match="autostart_delay must be between 0 and 84600" | |
| ): | |
| lab.set_autostart(enabled=True, delay=100000) |
tests/test_client_library_labs.py
Outdated
| } | ||
|
|
||
| if has_autostart: | ||
| topology["lab"]["autostart_config"] = { |
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.
| topology["lab"]["autostart_config"] = { | |
| topology["lab"]["autostart"] = { |
tests/test_client_library_labs.py
Outdated
| expected = {"enabled": False, "priority": None, "delay": None} | ||
|
|
||
| lab._import_lab(topology) | ||
| assert lab._autostart_config == expected |
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.
| assert lab._autostart_config == expected | |
| assert lab._autostart == expected |
tests/test_client_library_labs.py
Outdated
| lab._autostart_config = {"enabled": False, "priority": 100, "delay": 200} | ||
|
|
||
| properties = { | ||
| "title": "Updated Lab", | ||
| "autostart_config": {"enabled": True, "priority": 300}, | ||
| } | ||
|
|
||
| lab.update_lab_properties(properties) | ||
|
|
||
| assert lab._title == "Updated Lab" | ||
| assert lab._autostart_config == {"enabled": True, "priority": 300, "delay": 200} |
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.
| lab._autostart_config = {"enabled": False, "priority": 100, "delay": 200} | |
| properties = { | |
| "title": "Updated Lab", | |
| "autostart_config": {"enabled": True, "priority": 300}, | |
| } | |
| lab.update_lab_properties(properties) | |
| assert lab._title == "Updated Lab" | |
| assert lab._autostart_config == {"enabled": True, "priority": 300, "delay": 200} | |
| lab._autostart = {"enabled": False, "priority": 100, "delay": 200} | |
| properties = { | |
| "title": "Updated Lab", | |
| "autostart": {"enabled": True, "priority": 300}, | |
| } | |
| lab.update_lab_properties(properties) | |
| assert lab._title == "Updated Lab" | |
| assert lab._autostart == {"enabled": True, "priority": 300, "delay": 200} |
| @autostart_priority.setter | ||
| def autostart_priority(self, value: int | None) -> None: | ||
| """Set the autostart priority of the lab.""" | ||
| if value is not None and not (0 <= value <= 10000): |
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.
| if value is not None and not (0 <= value <= 10000): | |
| if not (value is None or 0 <= value <= 10000): |
| def autostart_priority(self, value: int | None) -> None: | ||
| """Set the autostart priority of the lab.""" | ||
| if value is not None and not (0 <= value <= 10000): | ||
| raise ValueError("autostart_priority must be between 0 and 10000") |
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.
| raise ValueError("autostart_priority must be between 0 and 10000") | |
| raise ValueError("autostart_priority must be between 0 and 10000, or None") |
| @autostart_delay.setter | ||
| def autostart_delay(self, value: int | None) -> None: | ||
| """Set the autostart delay of the lab.""" | ||
| if value is not None and (value < 0 or value > 84600): |
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.
I like this better, but the original is fine too except that it's 86400
| if value is not None and (value < 0 or value > 84600): | |
| if not (value is None or 0 <= value <= 86400): |
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.
@marpauli-cisco 84600 -> 86400 needs to be changed in API and UI as well
| "priority": None, | ||
| "delay": None, | ||
| }, | ||
| ) |
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.
this can be moved out of if/else and if autostart is not present, we shouldn't set it
| """ | ||
| url = self._url_for("labs") | ||
| body = {"title": title, "description": description, "notes": notes} | ||
|
|
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.
| "priority": priority, | ||
| "delay": delay, | ||
| } | ||
| self._set_property("autostart", self._autostart) |
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.
I made the same comment in the node staging, this method is imo sufficient (we can add a getter) and we don't need the individual getters/setters @marpauli-cisco thoughts?
tmikuska
left a comment
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.
the delay max needs to be changed and we shouldn't reset autostart on update if it's not present in the lab_dict (not sure if that's possible)
Added support for lab_autostart feature.
Currently using the already existing PATH /labs/{lab_id}