Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

flag.Duration and flag.DurationVar parse errors not handled properly #944

Closed
Dieterbe opened this issue Jun 14, 2018 · 7 comments
Closed

Comments

@Dieterbe
Copy link
Contributor

from a customer

we enabled index pruning (we were using 30d and that silently failed to parse as golang durations don’t support d unlike the other durations in MT)

flag.Duration and flag.DurationVar have no way to validate input, they just return 0
in some of these flags 0 is a valid input value, so in those cases we should use another input type..
where we do use the flag.Duration* types, we should validate that they are !=0

maybe for consistency we should use our own parsing everywhere

@Dieterbe Dieterbe added this to the 0.9.1 milestone Jun 14, 2018
@Dieterbe
Copy link
Contributor Author

flag.Duration and flag.DurationVar have no way to validate input

incorrect. though the api is kindof weird.
the durationvars method return the error, and you can have the flagset exit or panic when it happens.
see https://play.golang.org/p/qhoOi_e2r79
need to do some more research if we can make it work with globalconf.
related: rakyll/globalconf#21

@Dieterbe Dieterbe modified the milestones: 1.1, 1.0 Aug 17, 2018
@Dieterbe
Copy link
Contributor Author

I think the simplest and clearest approach is to just use stringvars everywhere and do an explicit parse step where we can easily check the error and fatal out if needed

@replay
Copy link
Contributor

replay commented Sep 14, 2018

I guess now that it's merged we can just reuse ConvertTimeout() (https://github.com/grafana/metrictank/blob/master/store/cassandra/cassandra.go#L114), although we might want to move it somewhere else if it becomes an often reused utility function

@Dieterbe
Copy link
Contributor Author

while we're at it: #485 (comment)

@deniszh
Copy link

deniszh commented Oct 1, 2018

I did same '30d' mistake too, maybe it should be mentioned in example config?

@deniszh
Copy link

deniszh commented Oct 1, 2018

Or let me know how can I help with fixing that issue ( I mean which exact help is wanted)

robert-milan added a commit that referenced this issue Oct 17, 2018
Change flag.Duration and flag.DurationVar to flag.StringVar for parsing
Add config.go to cassandra idx to align with cassandra store
Add variables to cassandra idx to avoid indirection in calls to config
Update docs and metrictank example configs to reflect changes

Duration configuration settings in cassandra idx will now accept the
following time units: s/sec/secs/second/seconds, m/min/mins/minute/minutes,
h/hour/hours, d/day/days, w/week/weeks, mon/month/months, y/year/years

First step in implementing flag.StringVar throughout all of metrictank to allow
easier parsing and control

See also: #944
robert-milan added a commit that referenced this issue Nov 14, 2018
add ability to implement custom validation for config values in cass idx
add config.go for cass idx configuration
update cass idx tests to use new config
update metrictank config and docs to show valid time units

update mt-index-cat to use new cass idx config
update mt-whisper-importer-writer to use new cass idx config

See also: #944
robert-milan added a commit that referenced this issue Nov 29, 2018
add ability to implement custom validation for config values in cass idx
add config.go for cass idx configuration
update cass idx tests to use new config
update metrictank config and docs to show valid time units

update mt-index-cat to use new cass idx config
update mt-whisper-importer-writer to use new cass idx config

See also: #944
Dieterbe added a commit that referenced this issue Dec 3, 2018
Dieterbe pushed a commit that referenced this issue Dec 14, 2018
add ability to implement custom validation for config values in cass idx
add config.go for cass idx configuration
update cass idx tests to use new config
update metrictank config and docs to show valid time units

update mt-index-cat to use new cass idx config
update mt-whisper-importer-writer to use new cass idx config

See also: #944
Dieterbe added a commit that referenced this issue Dec 14, 2018
@Dieterbe Dieterbe modified the milestones: 1.0, 0.11.0 Dec 14, 2018
@Dieterbe
Copy link
Contributor Author

fixed by #1141

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants