- Notifications
You must be signed in to change notification settings - Fork927
feat(cli): extend duration to longer units#15040
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
93e2e47617fda16ce2600950c056e29463002cfb2bc51cf20256c08512e2c2d7723ae9File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -46,7 +46,7 @@ func (r *RootCmd) tokens() *serpent.Command { | ||
| func (r *RootCmd) createToken() *serpent.Command { | ||
| var ( | ||
| tokenLifetimestring | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @defelmnq@johnstcn WDYT about defining a new type incoder/serpent, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Could be done as a follow-up! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I created afollow-up issue here - would definitely be a cool one to do indeed. | ||
| name string | ||
| user string | ||
| ) | ||
| @@ -63,8 +63,30 @@ func (r *RootCmd) createToken() *serpent.Command { | ||
| if user != "" { | ||
| userID = user | ||
| } | ||
| var parsedLifetime time.Duration | ||
| var err error | ||
| tokenConfig, err := client.GetTokenConfig(inv.Context(), userID) | ||
| if err != nil { | ||
| return xerrors.Errorf("get token config: %w", err) | ||
| } | ||
| if tokenLifetime == "" { | ||
| parsedLifetime = tokenConfig.MaxTokenLifetime | ||
| } else { | ||
| parsedLifetime, err = extendedParseDuration(tokenLifetime) | ||
| if err != nil { | ||
| return xerrors.Errorf("parse lifetime: %w", err) | ||
| } | ||
Comment on lines +78 to +81 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. suggestion: given that we have the token config available, we can check if the requested lifetime is greater than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah you're completely right - makes sense I just changed it a bit to handle the config fetched ! 👌 | ||
| if parsedLifetime > tokenConfig.MaxTokenLifetime { | ||
| return xerrors.Errorf("lifetime (%s) is greater than the maximum allowed lifetime (%s)", parsedLifetime, tokenConfig.MaxTokenLifetime) | ||
| } | ||
| } | ||
| res, err := client.CreateToken(inv.Context(), userID, codersdk.CreateTokenRequest{ | ||
| Lifetime:parsedLifetime, | ||
| TokenName: name, | ||
| }) | ||
| if err != nil { | ||
| @@ -82,8 +104,7 @@ func (r *RootCmd) createToken() *serpent.Command { | ||
| Flag: "lifetime", | ||
| Env: "CODER_TOKEN_LIFETIME", | ||
| Description: "Specify a duration for the lifetime of the token.", | ||
| Value: serpent.StringOf(&tokenLifetime), | ||
| }, | ||
| { | ||
| Flag: "name", | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2,6 +2,7 @@ package cli | ||
| import ( | ||
| "fmt" | ||
| "regexp" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
| @@ -181,6 +182,78 @@ func isDigit(s string) bool { | ||
| }) == -1 | ||
| } | ||
| // extendedParseDuration is a more lenient version of parseDuration that allows | ||
| // for more flexible input formats and cumulative durations. | ||
| // It allows for some extra units: | ||
| // - d (days, interpreted as 24h) | ||
| // - y (years, interpreted as 8_760h) | ||
| // | ||
| // FIXME: handle fractional values as discussed in https://github.com/coder/coder/pull/15040#discussion_r1799261736 | ||
| func extendedParseDuration(raw string) (time.Duration, error) { | ||
| var d int64 | ||
| isPositive := true | ||
| // handle negative durations by checking for a leading '-' | ||
| if strings.HasPrefix(raw, "-") { | ||
| raw = raw[1:] | ||
| isPositive = false | ||
| } | ||
| if raw == "" { | ||
| return 0, xerrors.Errorf("invalid duration: %q", raw) | ||
| } | ||
| // Regular expression to match any characters that do not match the expected duration format | ||
| invalidCharRe := regexp.MustCompile(`[^0-9|nsuµhdym]+`) | ||
| if invalidCharRe.MatchString(raw) { | ||
| return 0, xerrors.Errorf("invalid duration format: %q", raw) | ||
| } | ||
| // Regular expression to match numbers followed by 'd', 'y', or time units | ||
mtojek marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| re := regexp.MustCompile(`(-?\d+)(ns|us|µs|ms|s|m|h|d|y)`) | ||
| matches := re.FindAllStringSubmatch(raw, -1) | ||
| for _, match := range matches { | ||
| var num int64 | ||
| num, err := strconv.ParseInt(match[1], 10, 0) | ||
| if err != nil { | ||
| return 0, xerrors.Errorf("invalid duration: %q", match[1]) | ||
| } | ||
| switch match[2] { | ||
| case "d": | ||
| // we want to check if d + num * int64(24*time.Hour) would overflow | ||
| if d > (1<<63-1)-num*int64(24*time.Hour) { | ||
| return 0, xerrors.Errorf("invalid duration: %q", raw) | ||
| } | ||
| d += num * int64(24*time.Hour) | ||
| case "y": | ||
| // we want to check if d + num * int64(8760*time.Hour) would overflow | ||
| if d > (1<<63-1)-num*int64(8760*time.Hour) { | ||
| return 0, xerrors.Errorf("invalid duration: %q", raw) | ||
| } | ||
| d += num * int64(8760*time.Hour) | ||
| case "h", "m", "s", "ns", "us", "µs", "ms": | ||
| partDuration, err := time.ParseDuration(match[0]) | ||
| if err != nil { | ||
| return 0, xerrors.Errorf("invalid duration: %q", match[0]) | ||
| } | ||
| if d > (1<<63-1)-int64(partDuration) { | ||
| return 0, xerrors.Errorf("invalid duration: %q", raw) | ||
| } | ||
| d += int64(partDuration) | ||
| default: | ||
| return 0, xerrors.Errorf("invalid duration unit: %q", match[2]) | ||
| } | ||
| } | ||
| if !isPositive { | ||
| return -time.Duration(d), nil | ||
| } | ||
| return time.Duration(d), nil | ||
| } | ||
| // parseTime attempts to parse a time (no date) from the given string using a number of layouts. | ||
| func parseTime(s string) (time.Time, error) { | ||
| // Try a number of possible layouts. | ||
defelmnq marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -41,6 +41,50 @@ func TestDurationDisplay(t *testing.T) { | ||
| } | ||
| } | ||
| func TestExtendedParseDuration(t *testing.T) { | ||
| t.Parallel() | ||
| for _, testCase := range []struct { | ||
| Duration string | ||
| Expected time.Duration | ||
| ExpectedOk bool | ||
| }{ | ||
| {"1d", 24 * time.Hour, true}, | ||
| {"1y", 365 * 24 * time.Hour, true}, | ||
defelmnq marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| {"10s", 10 * time.Second, true}, | ||
| {"1m", 1 * time.Minute, true}, | ||
| {"20h", 20 * time.Hour, true}, | ||
mtojek marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| {"10y10d10s", 10*365*24*time.Hour + 10*24*time.Hour + 10*time.Second, true}, | ||
defelmnq marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| {"10ms", 10 * time.Millisecond, true}, | ||
defelmnq marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| {"5y10d10s5y2ms8ms", 10*365*24*time.Hour + 10*24*time.Hour + 10*time.Second + 10*time.Millisecond, true}, | ||
| {"10yz10d10s", 0, false}, | ||
Comment on lines +51 to +59 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. question: What happens if we try to parse a duration greater than
defelmnq marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| {"1µs2h1d", 1*time.Microsecond + 2*time.Hour + 1*24*time.Hour, true}, | ||
| {"1y365d", 2 * 365 * 24 * time.Hour, true}, | ||
| {"1µs10us", 1*time.Microsecond + 10*time.Microsecond, true}, | ||
| // negative related tests | ||
| {"-", 0, false}, | ||
| {"-2h10m", -2*time.Hour - 10*time.Minute, true}, | ||
| {"--10s", 0, false}, | ||
| {"10s-10m", 0, false}, | ||
| // overflow related tests | ||
| {"-20000000000000h", 0, false}, | ||
| {"92233754775807y", 0, false}, | ||
| {"200y200y200y200y200y", 0, false}, | ||
| {"9223372036854775807s", 0, false}, | ||
| } { | ||
| testCase := testCase | ||
| t.Run(testCase.Duration, func(t *testing.T) { | ||
| t.Parallel() | ||
| actual, err := extendedParseDuration(testCase.Duration) | ||
| if testCase.ExpectedOk { | ||
| require.NoError(t, err) | ||
| assert.Equal(t, testCase.Expected, actual) | ||
| } else { | ||
| assert.Error(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| func TestRelative(t *testing.T) { | ||
| t.Parallel() | ||
| assert.Equal(t, relative(time.Minute), "in 1m") | ||
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.