Bugfix: Retention period is being sent to the API as 0 - #5706
Conversation
It was failing with 0 context about which particular scenario failed and it wasn't logging the errors returned by the scenarios
|
Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message. |
| permissionsOptOut bool | ||
| devContainerPath string | ||
| idleTimeout time.Duration | ||
| retentionPeriod time.Duration |
There was a problem hiding this comment.
could you use a *time.Duration instead? using pointers is a common pattern for optional types
There was a problem hiding this comment.
I'm not sure if I can because the default argument of DurationVar from the pflag library is of type time.Duration so I can't pass in nil. I'm open to suggestions if you have an idea of how this could be refactored.
There was a problem hiding this comment.
This is true but the library provides what you need to define flag funcs for your own types: https://pkg.go.dev/github.com/spf13/pflag#Var
There was a problem hiding this comment.
Here's how we approach *string and *bool flag value receivers in the codebase already:
Lines 12 to 16 in ec12d27
While a pointer receiver is a valid way of differentiating between explicit empty value vs. flag not being set by the user, @reybard makes a good point that a custom flag receiver can be defined. If dealing with pointers proves to be convoluted, do not hesitate to make a custom value that wraps an underlying time.Duration value in an object that also keeps track of whether the flag was explicitly set or not. 👍
a3124a7 to
ac42656
Compare
I think this makes parsing the duration from it and supporting a nil-duration easier.
ac42656 to
8b4c859
Compare
reybard
left a comment
There was a problem hiding this comment.
very gopher to use the interface 
👍
| return nil | ||
| } | ||
|
|
||
| func (d *NullableDuration) Type() string { |
Co-authored-by: Jose Garcia <josebalius@github.com>
The initialized value of
time.Durationis 0. There is support fornilretention period at the API interface, but we missed support for this at the parameters. Thus, the API request is being made with0as the retention period.This PR switches the argument type to a string and treats an empty string as a no-retention-period scenario and attempts to parse the string as a duration if the string is non-empty.