-
Notifications
You must be signed in to change notification settings - Fork 165
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
util: put ParseDiskSpec
func to util
#3484
Conversation
Skipping CI for Draft Pull Request. |
f841e42
to
2098727
Compare
c2bc195
to
f865c17
Compare
f865c17
to
74f7b54
Compare
b2a4002
to
e853e0e
Compare
8e05ead
to
1c13af6
Compare
1c13af6
to
0ce5219
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a few nits
@@ -54,7 +54,7 @@ func testMountDisks(c cluster.TestCluster) { | |||
config := setupIgnitionConfig() | |||
|
|||
options := platform.MachineOptions{ | |||
AdditionalDisks: []string{"1024M", "1024M"}, | |||
AdditionalDisks: []string{"1G", "1G"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm tricky...if we were supporting the M
specifier before it may be safer to continue doing so.
It looks like all of the external tests use G
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be better, but we intend to get size as int (trim sufix G
) in the ParseDiskSpec func
:
- for qemu, need to re-add
G
as it needs string to be passed to qemu-img - for gcp and other cloud, need int, as the default is size in
G
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems a bit hard to do without re-implementing the suffix parsing here entirely which would be unfortunate.
I think this is OK as is!
0ce5219
to
894afa5
Compare
Refer to @jlebon's suggestion, see coreos#3477 (comment)
894afa5
to
246495c
Compare
Thanks @cgwalters and @jlebon for the review. |
Refer to @jlebon's suggestion, see #3477 (comment)