-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fix termination-time and possibility to use strtotime strings #59
Conversation
Thanks for finding the bug and adding this functionality @Miouge1! |
pkg/drivers/metal/metal.go
Outdated
mcnflag.StringFlag{ | ||
Name: "metal-ttl", | ||
Usage: "Set the Instance maximum Time To Live", | ||
EnvVar: "METAL_TTL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think metal-ttl
could be misunderstood to be a metal-api-request-ttl
or similar.
What do you think about extending the existing metal-termination-time
field with https://github.com/carmo-evan/strtotime? I think this would enable the functionality you are looking for (and then some) while continuing to offer the existing date parsing behavior (which, as you've pointed out, never worked).
https://github.com/carmo-evan/strtotime/blob/master/strtotime_test.go#L15-L94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to have metal-termination-time
support either a fixed time or duration.
I think time.Duration
is convenient enough for most duration 1h
, 1d
, 90d
. strtotime
can be an additional convenience for a more flexible date format, especially when used in the command line if you don't mind the additional dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time.ParseDuration
only handles hours and smaller. https://play.golang.org/p/RouRQvFsLka
The dependency is thin (one visual page scan (lgtm), and no external dependencies). The licenses (BSD-3 and MIT) are amicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmarhee thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miouge1 let's reuse metal-termination-time
and change the parser out. with the strtotime parser your "1h","90d" snippets would need to be "1 hour", "90 day", etc, but we also get support for the existing format and added parsing abilities: "next week", "tomorrow", etc.
If you prefer, I can make these changes to your branch (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I've updated the PR code and title to reflect the changes.
The neat thing with strtotime is that it is compatible with the previously intended format (2006-01-02T15:04:05.000Z
).
I have tested this with:
--metal-termination-time="10 minutes"
--metal-termination-time="next week"
--metal-termination-time="yesterday"
, returns an error as expected:Error setting machine configuration from flags provided: --metal-termination-time cannot be in the past
--metal-termination-time="2006-01-02T15:04:05.000Z"
, returns an error as expected:Error setting machine configuration from flags provided: --metal-termination-time cannot be in the past
--metal-termination-time="foobar"
, returns an error as expected:Error setting machine configuration from flags provided: strtotime: Unrecognizable input: "foobar"
I propose to add a "time to live" option set as a duration added to creation time.
From a user experience point of view it's often easier to set
--metal-ttl=6h
than calculating the appropriate timesampt and setting this as--metal-termination-time
Also I noticed that the
TerminationTime
was not set in theDeviceCreateRequest
therefore the--metal-termination-time
is ignored, which results in machines not being deleted as expected.