Bugfix: Retention period is being sent to the API as 0

[greggroth]

The initialized value of time.Duration is 0. There is support for nil retention period at the API interface, but we missed support for this at the parameters. Thus, the API request is being made with 0 as 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.

[cliAutomation]

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.

[markphelps]

could you use a *time.Duration instead? using pointers is a common pattern for optional types

[greggroth]

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.

[reybard]

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

https://pkg.go.dev/github.com/spf13/pflag#Value

[mislav]

Here's how we approach *string and *bool flag value receivers in the codebase already:

cli/pkg/cmdutil/flags.go

Lines 12 to 16 in ec12d27

	// NilStringFlag defines a new flag with a string pointer receiver. This is useful for differentiating
	// between the flag being set to a blank value and the flag not being passed at all.
	func NilStringFlag(cmd *cobra.Command, p **string, name string, shorthand string, usage string) *pflag.Flag {
	return cmd.Flags().VarPF(newStringValue(p), name, shorthand, usage)
	}

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

[reybard]

very gopher to use the interface 👍

[josebalius]

Why is this needed?

[greggroth]

It's part of the Value interface