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

fix(vm): Add parser for CustomEFIDisk #208

Merged
merged 10 commits into from
Jan 18, 2023
Merged

fix(vm): Add parser for CustomEFIDisk #208

merged 10 commits into from
Jan 18, 2023

Conversation

kaje783
Copy link
Contributor

@kaje783 kaje783 commented Jan 16, 2023

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

Add parser to use efi disk for UEFI

Closes #202

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Thanks again @kaje783! 👍🏻

Just a minor feedback about the size parsing.

Comment on lines 1449 to 1450
size := strings.ReplaceAll(v[1], "M", "")
iv, err := strconv.Atoi(size)
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better to use parseDiskSize(string) from utils.go, for consistency.

Suggested change
size := strings.ReplaceAll(v[1], "M", "")
iv, err := strconv.Atoi(size)
iv, err := parseDiskSize(v[1])

@kaje783
Copy link
Contributor Author

kaje783 commented Jan 17, 2023

@bpg the function parseDiskSize(&string) is much better for this, i have changed it.
Thanks.

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Cool, I've added unit tests for the func and refactored the code a bit to address linter errors and remove duplication.

LGTM! 🚀

@bpg bpg merged commit b539aab into bpg:main Jan 18, 2023
@ghost ghost mentioned this pull request Jan 18, 2023
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.

cannot unmarshal string into Go struct field VirtualEnvironmentVMGetResponseData.data.efidisk0
2 participants