Skip to content

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Oct 16, 2024
Merged

feat(cli): extend duration to longer units #15040

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
93e2e47
feat(cli): extend duration to longer units
defelmnq Oct 10, 2024
617fda1
feat(cli): call codersk to get default token lifetime
defelmnq Oct 10, 2024
6ce2600
fix: golden files and docs
defelmnq Oct 10, 2024
950c056
feat(cli): extend duration to longer units
defelmnq Oct 14, 2024
e294630
Merge remote-tracking branch 'origin/feat/improve-duration-flags' int…
defelmnq Oct 14, 2024
02cfb2b
feat(cli): improve long duration testing and negative values
defelmnq Oct 14, 2024
c51cf20
feat(cli): improve long duration testing and negative values
defelmnq Oct 14, 2024
256c085
feat(cli): improve long duration testing and negative values
defelmnq Oct 14, 2024
12e2c2d
feat(cli): improve long duration testing and negative values
defelmnq Oct 14, 2024
7723ae9
Merge remote-tracking branch 'origin/main' into feat/improve-duration…
defelmnq Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/testdata/coder_tokens_create_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ USAGE:
Create a token

OPTIONS:
--lifetime duration, $CODER_TOKEN_LIFETIME (default: 720h0m0s)
--lifetime string, $CODER_TOKEN_LIFETIME
Specify a duration for the lifetime of the token.

-n, --name string, $CODER_TOKEN_NAME
Expand Down
25 changes: 21 additions & 4 deletions cli/tokens.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (r *RootCmd) tokens() *serpent.Command {

func (r *RootCmd) createToken() *serpent.Command {
var (
tokenLifetime time.Duration
tokenLifetime string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@defelmnq @johnstcn WDYT about defining a new type in coder/serpent, time.LongDuration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be done as a follow-up!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a follow-up issue here - would definitely be a cool one to do indeed.

name string
user string
)
Expand All @@ -63,8 +63,26 @@ func (r *RootCmd) createToken() *serpent.Command {
if user != "" {
userID = user
}

var parsedLifetime time.Duration
var err error

if tokenLifetime == "" {
tokenConfig, err := client.GetTokenConfig(inv.Context(), userID)
if err != nil {
return xerrors.Errorf("get token config: %w", err)
}

parsedLifetime = tokenConfig.MaxTokenLifetime
} else {
parsedLifetime, err = extendedParseDuration(tokenLifetime)
if err != nil {
return xerrors.Errorf("parse lifetime: %w", err)
}
Comment on lines +78 to +81
Copy link
Member

Choose a reason for hiding this comment

The 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 MaxTokenLifetime client-side. This may not be completely necessary though, as the API will disallow creating tokens with lifetime greater than MaximumTokenDuration. Dealer's choice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ! 👌

}

res, err := client.CreateToken(inv.Context(), userID, codersdk.CreateTokenRequest{
Lifetime: tokenLifetime,
Lifetime: parsedLifetime,
TokenName: name,
})
if err != nil {
Expand All @@ -82,8 +100,7 @@ func (r *RootCmd) createToken() *serpent.Command {
Flag: "lifetime",
Env: "CODER_TOKEN_LIFETIME",
Description: "Specify a duration for the lifetime of the token.",
Default: (time.Hour * 24 * 30).String(),
Value: serpent.DurationOf(&tokenLifetime),
Value: serpent.StringOf(&tokenLifetime),
},
{
Flag: "name",
Expand Down
33 changes: 33 additions & 0 deletions cli/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,39 @@ func isDigit(s string) bool {
}) == -1
}

// extendedParseDuration is a more lenient version of parseDuration that allows
// for more flexible input formats.
// It allows for some extra units:
// - d (days)
// - y (years)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may make sense to explicitly note that this will not take into account things like DST or leap years or all that other assorted wibbly wobbley timey wimey stuff.

Suggested change
// - d (days)
// - y (years)
// - d (days, interpreted as 24 hours)
// - y (years, interpreted as 8_760 hours)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first though was about leap years too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - indeed we can document it. About the behavior are we good to keep it this way ? I felt like it can quickly become complicated with time saving, leap years etc.. otherwise - maybe we can do it in the follow-up PR in serpent if we want to use it everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, definitely not a must-have

Copy link
Member

@johnstcn johnstcn Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with a naive definition of days and years here. Adding DST / leap year awareness adds a lot of complications, as you need both a timezone and a relative time. We can add a follow-up issue if we think it's necessary though.

func extendedParseDuration(raw string) (time.Duration, error) {
var duration time.Duration
var err error

switch {
case strings.HasSuffix(raw, "d"):
nbDays, err := strconv.Atoi(strings.TrimSuffix(raw, "d"))
if err != nil {
return 0, xerrors.Errorf("invalid duration: %q", raw)
}
duration = time.Duration(nbDays) * 24 * time.Hour
case strings.HasSuffix(raw, "y"):
nbYears, err := strconv.Atoi(strings.TrimSuffix(raw, "y"))
if err != nil {
return 0, xerrors.Errorf("invalid duration: %q", raw)
}
duration = time.Duration(nbYears) * 365 * 24 * time.Hour
default:
duration, err = time.ParseDuration(raw)
}

if err != nil {
return 0, xerrors.Errorf("invalid duration: %q", raw)
}
return duration, nil
}

// parseTime attempts to parse a time (no date) from the given string using a number of layouts.
// 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.
Expand Down
28 changes: 28 additions & 0 deletions cli/util_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,34 @@ 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},
{"10s", 10 * time.Second, true},
{"1m", 1 * time.Minute, true},
{"20h", 20 * time.Hour, true},
{"10y10d10s", 0, false},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users will expect durations like 1y1d to work here. It shouldn't be a huge lift to enable this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @johnstcn I tried to play with time.ParseDuration golang function to mimic the behavior as much as possible - and changed to indeed modify what you've raised.

} {
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")
Expand Down
3 changes: 1 addition & 2 deletions docs/reference/cli/tokens_create.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading