-
Notifications
You must be signed in to change notification settings - Fork 60
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
flagext: Add support for using Bytes as a CLI flag #302
Conversation
Adds support for use as a CLI flag in addition to the existing YAML serialization. Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
} | ||
|
||
func Test_Bytes_Set(t *testing.T) { | ||
for _, tcase := range []struct { |
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.
[nit] Given str
is the input value, I'd put it first, as then the test cases read "this is the input, and this is the expected output"
flagext/bytes_test.go
Outdated
|
||
func Test_Bytes_UnmarshalYAML(t *testing.T) { | ||
for _, tcase := range []struct { | ||
value uint64 |
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.
[nit] Same as above - given str
is the input value, I'd put it first, as then the test cases read "this is the input, and this is the expected output"
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.
Not sure if this is really necessary given we're using a library for the conversions, but it might be good to include some test cases that aren't exactly 1 GB / MB / KB (eg. 1500 bytes or 2.3 GB)
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.
Will do 👍
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.
Turns out the library will format things like that in a very precise yet ugly way: 1536 * 1024 * 1024
becomes 1GiB512MiB
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.
I guess the good news is that we'll almost never go from int
-> string
. This would mostly be used for going the other direction, taking user input of string
and turning it into an int
.
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 I think that's fine, as long as it can handle a user providing something like 2.3 GiB
.
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.
LGTM (modulo Charles' comments). Thanks!
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
What this PR does:
Adds support for use as a CLI flag in addition to the existing YAML serialization.
Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]