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

Add disks management commands #1716

Merged
merged 8 commits into from
Sep 10, 2020
Merged

Add disks management commands #1716

merged 8 commits into from
Sep 10, 2020

Conversation

romasku
Copy link
Contributor

@romasku romasku commented Sep 3, 2020

Part of #1715

@romasku romasku force-pushed the rs/disk-management-api branch from 606278f to 3948994 Compare September 3, 2020 12:44
@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #1716 into master will decrease coverage by 1.06%.
The diff coverage is 98.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1716      +/-   ##
==========================================
- Coverage   87.18%   86.11%   -1.07%     
==========================================
  Files          59       62       +3     
  Lines        8849     8969     +120     
  Branches     1440     1445       +5     
==========================================
+ Hits         7715     7724       +9     
- Misses        890      985      +95     
- Partials      244      260      +16     
Flag Coverage Δ
#e2e 61.46% <98.33%> (-3.36%) ⬇️
#unit 77.26% <87.60%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
neuromation/cli/job.py 81.50% <ø> (-2.44%) ⬇️
neuromation/cli/secrets.py 100.00% <ø> (ø)
neuromation/cli/disks.py 94.73% <94.73%> (ø)
neuromation/api/client.py 98.88% <100.00%> (+0.06%) ⬆️
neuromation/api/config.py 91.86% <100.00%> (+0.09%) ⬆️
neuromation/api/disks.py 100.00% <100.00%> (ø)
neuromation/cli/formatters/disks.py 100.00% <100.00%> (ø)
neuromation/cli/main.py 74.13% <100.00%> (-1.27%) ⬇️
neuromation/cli/image.py 55.73% <0.00%> (-44.27%) ⬇️
neuromation/cli/tty_utils.py 50.00% <0.00%> (-11.12%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dfcd39...03ad2d8. Read the comment docs.

@romasku romasku changed the title Add disks management to API Add disks management commands Sep 3, 2020
@romasku romasku force-pushed the rs/disk-management-api branch from 53edba9 to 9e9036c Compare September 9, 2020 14:00
Comment on lines 22 to 29
if disk.storage >= 1024 ** 3:
storage_str = f"{disk.storage / (1024 ** 3):.2f}G"
elif disk.storage >= 1024 ** 2:
storage_str = f"{disk.storage / (1024 ** 2):.2f}M"
elif disk.storage >= 1024:
storage_str = f"{disk.storage / 1024:.2f}K"
else:
storage_str = str(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.

Can it use cli.utils.format_size()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure, thanks for pointing out.

@romasku romasku force-pushed the rs/disk-management-api branch from d13b506 to e87979f Compare September 9, 2020 14:51

:return: Newly created disk info (:class:`Disk`)

.. comethod:: get(disk_id: str) -> None
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it return?

Comment on lines 40 to 42
status=next(
status for status in Disk.Status if status.value == payload["status"]
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Disk.Status(payload["status"])?

storage: int # In bytes
owner: str
status: "Disk.Status"
_cluster: str
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it underscored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I thought that this is some kind of internal data (as it isn't returned from server), but now I see no reason to underscore this field.

storage: int # In bytes
owner: str
status: "Disk.Status"
cluster: str
Copy link
Contributor

Choose a reason for hiding this comment

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

In all other classes (_ConfigData, JobDescription, RemoteImage, _QuotaInfo) we use attributes named cluster_name. Name cluster is never used. It is rather by accident, but it would be better to continue the tradition.

Copy link
Contributor

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

👍

@romasku romasku merged commit 4588c7f into master Sep 10, 2020
@romasku romasku deleted the rs/disk-management-api branch September 10, 2020 08:41
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