NotificationsYou must be signed in to change notification settings
Fork927
Star10.1k

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

Jump to bottom

Merged

defelmnq merged 10 commits intomainfromfeat/improve-duration-flags

Oct 16, 2024

Merged

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

defelmnq merged 10 commits intomainfromfeat/improve-duration-flags

Oct 16, 2024

Conversation

Copy link

Contributor

defelmnq commentedOct 10, 2024•
edited
Loading

This PR is a proposal to improve the situation described in#14750

For some precise commands - we would like to be able to use durations bigger than hours, minutes..

This PR extends the Duration proposed by Go with :

d - a day or 24hours.
y - a year or 365 days.

I also removed the default value for lifetime and instead fetch the maxLifetime value from codersdk - so by default if no value set we use the value defined in the config.

feat(cli): extend duration to longer units

93e2e47

defelmnq self-assigned this

Oct 10, 2024

defelmnq requested review fromdannykopping andEmyrk

October 10, 2024 22:46

defelmnqand others added2 commits

October 11, 2024 01:30

feat(cli): call codersk to get default token lifetime

617fda1

fix: golden files and docs

6ce2600

defelmnq requested a review frommtojek

October 10, 2024 23:47

johnstcn reviewed

Oct 11, 2024

View reviewed changes

cli/util_internal_test.go Outdated

		{"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

ContributorAuthor

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.

defelmnq added2 commits

October 14, 2024 02:01

feat(cli): extend duration to longer units

950c056

Merge remote-tracking branch 'origin/feat/improve-duration-flags' int…

e294630

…o feat/improve-duration-flags

defelmnq requested a review fromjohnstcn

October 14, 2024 00:05

Copy link

Member

matifali commentedOct 14, 2024•
edited
Loading

This is excellent@defelmnq. I suggest we stay uniform and check and update all durations of input throughput the product to support the string format across CLI/API/frontend.

It doesn't have to be in this PR and can be tracked as a separate issue.

johnstcn reviewed

Oct 14, 2024

View reviewed changes

cli/util.go Outdated

Comment on lines 188 to 189

		// - 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

ContributorAuthor

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

mtojekOct 14, 2024

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

johnstcnOct 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.

cli/util_internal_test.go

Comment on lines +51 to +59

		{"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", 1036524time.Hour + 1024time.Hour + 10time.Second, true},
		{"10ms", 10 * time.Millisecond, true},
		{"5y10d10s5y2ms8ms", 1036524time.Hour + 1024time.Hour + 10time.Second + 10*time.Millisecond, true},
		{"10yz10d10s", 0, false},

Copy link

Member

johnstcnOct 14, 2024

Choose a reason for hiding this comment

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

question: What happens if we try to parse a duration greater than2540400h10m10.000000000s (e.g.1000y)?
(see:https://github.com/golang/go/blob/master/src/time/time.go#L954C21-L954C45)

cli/tokens.go

Comment on lines +78 to +81

		parsedLifetime, err = extendedParseDuration(tokenLifetime)
		if err != nil {
		return xerrors.Errorf("parse lifetime: %w", err)
		}

Copy link

Member

johnstcnOct 14, 2024

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

Copy link

ContributorAuthor

defelmnqOct 14, 2024

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

cli/util_internal_test.goShow resolvedHide resolved

mtojek reviewed

Oct 14, 2024

View reviewed changes

cli/tokens.go

		@@ -46,7 +46,7 @@ func (r RootCmd) tokens() serpent.Command {

		func (r RootCmd) createToken() serpent.Command {
		var (
		tokenLifetimetime.Duration
		tokenLifetimestring

Copy link

Member

mtojekOct 14, 2024

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 incoder/serpent,time.LongDuration?

Copy link

Member

johnstcnOct 14, 2024

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

ContributorAuthor

defelmnqOct 14, 2024

Choose a reason for hiding this comment

The 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.

cli/util_internal_test.goShow resolvedHide resolved

cli/util.goShow resolvedHide resolved

cli/util.go Outdated

Comment on lines 188 to 189

		// - d (days)
		// - y (years)

Copy link

Member

mtojekOct 14, 2024

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 :)

feat(cli): improve long duration testing and negative values

02cfb2b

Emyrk reviewed

Oct 14, 2024

View reviewed changes

Copy link

Member

Emyrk left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I see this being valuable outside this 1 case.

serpent.Duration is used in a few places inDeploymentValues. Can we make this a new time, called likeserpent.ExtendedDuration?

Or maybe more controversial, just include this intoserpent.Duration itself?

defelmnq added3 commits

October 14, 2024 18:19

feat(cli): improve long duration testing and negative values

c51cf20

feat(cli): improve long duration testing and negative values

256c085

feat(cli): improve long duration testing and negative values

12e2c2d

defelmnq requested review frommtojek andjohnstcn

October 14, 2024 16:26

Copy link

ContributorAuthor

defelmnq commentedOct 14, 2024

Hey@mtojek @johnstcn thanks a lot for the input here - really appreciate.

Since the last review, here are globally the points I tried to change and improve :

Handling & tests for negative values and overflow.
Stick to go time package to add 'segments' (2h2h10m is valid and equals 4h10m)
Verifying client-side if the defined token lifetime is valid compared to the one fetched in the config.

What I would love to do as a follow-up :