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.

Sign up for GitHub

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

Jump to bottom

feat(vm,lxc): Improved support for different disk size units #326

Merged
merged 5 commits into from
May 10, 2023
Merged

feat(vm,lxc): Improved support for different disk size units #326

merged 5 commits into from
May 10, 2023

Conversation

zeddD1abl0
Copy link
Contributor

@zeddD1abl0 zeddD1abl0 commented May 7, 2023 •
edited by bpg
Loading

Contributor's Note

Please mark the following items with an [x] if they apply to your PR.
Leave the [ ] if they are not applicable, or if you have not completed the item.

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated templates in /examples for any new or updated resources / data sources.
  • I have ran make examples to verify that the change works as expected.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates to #86 as previously discussed.
Closes #314

bpg added 3 commits May 8, 2023 21:00
@bpg
Merge branch 'main' into 86-take-2-add-sizing-regex
b841e0e
@bpg
add unit tests
777b737 
make the size unit mandatory as it was before
remove support for floating point strings
remove support for optional space(s) between size number and units
@bpg
refactoring to DiskSize datatype
c810feb 
- move the code under types/disk_size.go
- add unit tests
- add "to string" conversion
- refactor all API types to use `DiskSize` type instead of string/int, and use JSON marshalling / unmarshalling where possible
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 10, 2023
@bpg
Copy link
Owner

bpg commented May 10, 2023

Hey @zeddD1abl0 👋🏼
I did some research and found this piece of code responsible for the "disk-size" data type conversion in PVE that we're trying to replicate: https://github.com/proxmox/pve-common/blob/8328617d06d60160660393faea10569d06ae0c66/src/PVE/JSONSchema.pm#L829

So I took a liberty to update your PR to introduce a new DiskSize data type and a similar logic to convert to/from strings.
As you may notice, I reverted back to you original idea of storing everything as bytes, and then use conversion to "default" gigabytes where necessary.

@bpg bpg changed the title Convert to Regex for size conversion feat(vm,lxc): Improved support for different disk size units May 10, 2023
@bpg bpg mentioned this pull request May 10, 2023
Closed
@zeddD1abl0
Copy link
Contributor Author

I like it. Can't wait to see it in action.

@bpg bpg merged commit 4be9914 into bpg:main May 10, 2023
@ghost ghost mentioned this pull request May 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpg bpg bpg approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Efidisk cannot parse disk size "528K"
2 participants
@zeddD1abl0 @bpg