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

feat: add disk storage to session launchers #590

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

leafty
Copy link
Member

@leafty leafty commented Dec 20, 2024

Closes #589.

Add a new disk_storage field to session launchers so that a custom disk size can be persisted.

Note: the disk storage size is not validated when saved.

See also: SwissDataScienceCenter/renku-ui#3463
Deployment: https://renku-ci-ui-3463.dev.renku.ch/

@coveralls
Copy link

coveralls commented Dec 20, 2024

Pull Request Test Coverage Report for Build 12478844553

Details

  • 23 of 23 (100.0%) changed or added relevant lines in 6 files are covered.
  • 9 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 86.83%

Files with Coverage Reduction New Missed Lines %
components/renku_data_services/base_api/error_handler.py 1 89.47%
components/renku_data_services/base_api/misc.py 1 98.36%
components/renku_data_services/notebooks/api/classes/k8s_client.py 7 63.58%
Totals Coverage Status
Change from base Build 12464122126: -0.02%
Covered Lines: 15230
Relevant Lines: 17540

💛 - Coveralls

@leafty leafty marked this pull request as ready for review December 24, 2024 07:57
@leafty leafty requested a review from a team as a code owner December 24, 2024 07:57
Copy link
Contributor

@m-alisafaee m-alisafaee left a comment

Choose a reason for hiding this comment

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

Thanks Flora! Please see my comments.

@@ -118,9 +119,11 @@ def validate_session_launcher_patch(
resource_class_id = RESET
else:
resource_class_id = patch.resource_class_id
disk_storage = RESET if "disk_storage" in data_dict and data_dict["disk_storage"] is None else patch.disk_storage
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: What if we don't want to change the disk storage in the PATCH? If we don't set it, the value will be None and it causes the disk storage to be reset (in the followup code).

Copy link
Member Author

Choose a reason for hiding this comment

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

A value of None does NOT reset the value during the patch operation.

@@ -451,6 +455,7 @@ async def test_patch_session_launcher(
assert res.json.get("name") == patch_payload["name"]
assert res.json.get("description") == patch_payload["description"]
assert res.json.get("resource_class_id") == patch_payload["resource_class_id"]
assert res.json.get("disk_storage") == 3
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: We should add a test to check disk storage isn't modified if it isn't sent in the PATCH.

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.

Persist the requested disk size for session launchers
3 participants