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

Fix GCP disk size conversion #99

Merged
merged 3 commits into from
May 24, 2024
Merged

Fix GCP disk size conversion #99

merged 3 commits into from
May 24, 2024

Conversation

paulajulve
Copy link
Contributor

Turns out that the GB units Google uses in their API responses are actually binary GB, aka, GiB. So the conversion to bytes needs a different factor.

Relevant docs are linked in the comment over the function, for future reference.

@paulajulve paulajulve self-assigned this May 23, 2024
Pokom
Pokom previously approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@Pokom Pokom left a comment

Choose a reason for hiding this comment

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

Might even be worthwhile to implement tests to ensure that we're always doing the conversion properly 😅

inkel
inkel previously approved these changes May 23, 2024
View reviewed changes
Copy link
Collaborator

@inkel inkel left a comment

Choose a reason for hiding this comment

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

LGTM

gcp/disk.go Outdated Show resolved Hide resolved
jjo
jjo previously approved these changes May 23, 2024
View reviewed changes
gcp/disk.go Outdated Show resolved Hide resolved
@paulajulve paulajulve dismissed stale reviews from jjo, inkel, and Pokom via 83425be May 23, 2024 16:33
@paulajulve paulajulve force-pushed the paula/fix-gcp-disk-size-conversion branch from 07fbb62 to 83425be Compare May 23, 2024 16:33
@paulajulve
Copy link
Contributor Author

paulajulve commented May 23, 2024 •
edited
Loading

Sorry, I the automation undid all the approves 💩

Since we're only using 2^30 now, I've centralised the constant in the disk.go file and used it all over. And I've added tests for all 3 CSPs for the size both in GB and bytes. I didn't think it was worth adding that test to the interface, since what we want to guarantee is that the logic is sound within each specific provider, and that in turn already guarantees the interface.

@paulajulve
Copy link
Contributor Author

Dammit! I broke the tests 😅 brb

@paulajulve paulajulve force-pushed the paula/fix-gcp-disk-size-conversion branch from 83425be to b79aecc Compare May 23, 2024 16:38
@paulajulve paulajulve requested review from jjo, inkel and Pokom May 23, 2024 16:39
inkel
inkel previously approved these changes May 23, 2024
View reviewed changes
@paulajulve paulajulve merged commit a251b71 into main May 24, 2024
3 checks passed
@paulajulve paulajulve deleted the paula/fix-gcp-disk-size-conversion branch May 24, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inkel inkel inkel approved these changes

@the-it the-it Awaiting requested review from the-it

@nikimanoledaki nikimanoledaki Awaiting requested review from nikimanoledaki

@logyball logyball Awaiting requested review from logyball

@jjo jjo Awaiting requested review from jjo

@Pokom Pokom Awaiting requested review from Pokom

Assignees

@paulajulve paulajulve

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paulajulve @jjo @inkel @Pokom