Skip to content

web: use seconds in max TTL input #3576

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 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 5 commits into from
Aug 22, 2022
Merged

web: use seconds in max TTL input #3576

merged 5 commits into from
Aug 22, 2022

Conversation

ammario
Copy link
Member

@ammario ammario commented Aug 19, 2022

Milliseconds are more difficult to deal with due to
all of the zeros.

@ammario ammario requested a review from a team as a code owner August 19, 2022 03:04
@ammario ammario requested a review from BrunoQuaresma August 19, 2022 03:04
@ammario ammario force-pushed the max-ttl branch 5 times, most recently from b7fcf03 to 92cf9a1 Compare August 19, 2022 03:10
@ammario
Copy link
Member Author

ammario commented Aug 19, 2022

@BrunoQuaresma, I'm pretty new to TS and our frontend codebase. If I did anything in a suboptimal way, please let me know.

Milliseconds are more difficult to deal with due to
all of the zeros.

Also, describe this feature as "auto-stop" to be
consistent with our Workspace page UI and CLI. "ttl"
is our backend lingo which should eventually be updated.
@ammario ammario enabled auto-merge (squash) August 19, 2022 04:22
@BrunoQuaresma
Copy link
Collaborator

I think we could do better, and set this as minutes, what do you think?

@BrunoQuaresma
Copy link
Collaborator

@ammario I made the conversion in the handlers, so we don't have to add extra logic to map BE errors to a diff field, and vice-versa.

@ammario
Copy link
Member Author

ammario commented Aug 20, 2022

@BrunoQuaresma — makes sense. Thanks for showing me how this is done. Plz approve and then it can merge.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I'm all in favour of this! I think minutes is the lowest useful granularity of time with autostart though.

Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

I like this as well! Approving on behalf of FE to help you move this forward

@ammario
Copy link
Member Author

ammario commented Aug 22, 2022

I'm all in favour of this! I think minutes is the lowest useful granularity of time with autostart though.

I agree. Ideally the frontend accepts the same input as the CLI so there's so no truncation.

I thought this PR was a step forward but not ideal.

@ammario ammario merged commit 6fde537 into main Aug 22, 2022
@ammario ammario deleted the max-ttl branch August 22, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants