Replace all remaining time.ParseDurations with parseutil.ParseDurationSeconds #21357
Conversation
VioletHynes
changed the title
| @@ -989,33 +989,3 @@ func (d *timeValue) Get() interface{} { return *d.target } | |||
| func (d *timeValue) String() string { return (*d.target).String() } | |||
| func (d *timeValue) Example() string { return "time" } | |||
| func (d *timeValue) Hidden() bool { return d.hidden } | |||
|
|
|||
| // -- helpers | |||
There was a problem hiding this comment.
These three functions were unused. I found them as part of my search for time.ParseDuration, and thought it better to remove them.
builtin/credential/aws/path_login.go
Outdated
| @@ -19,6 +19,8 @@ import ( | |||
| "strings" | |||
| "time" | |||
|
|
|||
| "github.com/hashicorp/go-secure-stdlib/parseutil" | |||
|
|
|||
There was a problem hiding this comment.
Would it be good to remove this extra line?
There was a problem hiding this comment.
Good call, it was added by my IDE but it definitely doesn't look correct there, looking at the full file. I'll fix these!
There was a problem hiding this comment.
The change itself looks good. I notice almost every file modified though included extra lines in the imports. I wonder if you'd mind collapsing all of those empty lines so that we have 2 import groups in each of the modified files?
I also wonder if it's worth introducing a semgrep rule to prevent future uses of time.ParseDuration. Thoughts?
Yeah! Apologies, my IDE took the liberty of adding these, but they should be fixed now.
This is a good idea, and there's no worries about preventing it from applying to older code since I just removed it from older code. I'll have a look to see how hard this would be to add |
There was a problem hiding this comment.
@raskchanky Added a semgrep rule here! Thanks for the suggestion. As part of CI, we run all configs in the ci directory, which should include this one. We get no findings on the existing codebase, but hits like this when I add something in to test it:
../../physical/raft/raft.go
time-parse-duration
Usage of time.ParseDuration. Use parseutil.ParseDurationSeconds,
instead!
878┆ time.ParseDuration("abc")
There was a problem hiding this comment.
Awesome! Thanks for doing that 🙂
VioletHynes
deleted the
violethynes/change-all-parse-duration-to-parse-duration-seconds
branch
Vault's duration string format supports a strict superset of options compared to Go's duration string format: https://developer.hashicorp.com/vault/docs/concepts/duration-format
In particular, we support days, e.g.
5d, whereastime.ParseDurationdoes not.We use
parseutil.ParseDurationSecondseverywhere in the code we can, to support our advertised duration format and to be consistent with ourselves. The majority of our code already usesparseutil.ParseDurationSeconds, but there were a few places I found thattime.ParseDurationwas still being used as part of poking around, so I thought I'd fix them all at once.I'm not worried about the fact this change changes a lot of files.
ParseDurationSecondsis battle-tested, and there's no valid duration thattime.ParseDurationsupports that we don't also support.