switched interval to string in order to support a time+unit format#4903
Conversation
✅ Deploy Preview for kubeapps-dev canceled.Built without sensitive environment variables
|
antgamdia
left a comment
antgamdia
left a comment
There was a problem hiding this comment.
Thanks for both the thorough analysis at #4869 (comment) and the changes herein proposed!
However, I'm quite a skeptic when it comes to using a non-standard time format (in the past this has caused me several headaches 😅 ). So, I'd like to hear your opinion on moving towards an ISO 8601-compliant format to represent time duration instead of relying on the go built-in library duration format.
| * // Comment attached to moo. | ||
| * // | ||
| * // Another line attached to qux. | ||
| * optional double qux = 4; | ||
| * // Another line attached to moo. | ||
| * optional double moo = 4; | ||
| * | ||
| * // Detached comment for corge. This is not leading or trailing comments | ||
| * // to qux or corge because there are blank lines separating it from | ||
| * // to moo or corge because there are blank lines separating it from |
There was a problem hiding this comment.
It seems you're using a different ts-proto generator version. In #4902 I've upgraded some deps, so, if you're using a more recent version, there's no problem.
There was a problem hiding this comment.
i might actually use an older version.
the ts_proto is not coming from go.mod but from the dashboard node_modules, and i don't remember updating the libraries with yarn in some time, so my local machine might be out of date.
I will try do refresh my branch and yarn on my machine and regenerate.
There was a problem hiding this comment.
i refreshed to main, and ran yarn install to sync my machine. the ts_proto was at version 1.15.4 as declared in yarn.lock.
if i run buf generate, i get the same result.
| if d, err := time.ParseDuration(duration); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
I wonder if it is the proper way to encode the information. I mean, when sending it over the wire, we would be using a non-standard format for dealing with time durations.
I must reckon that sth like 1m30s is way more human-readable than PT11M30S... but the latter is an ISO 8601 duration format, meaning it is interoperable between major time libraries in both js and go code.
There was a problem hiding this comment.
If we were developing our own REST api, i would agree with you.
But here we are not, we are just creating a wrapper API around data that have existing schema and data format.
The duration value comes from the flux/carvel CR. It would be confusing to users to see the repository in the UI show PT24H but then see 24h in the CR.
It seems that kubernetes is using this simpler duration format, i don't think we should deviate from it.
There was a problem hiding this comment.
Yes, I agree it differs from what the Flux/Carvel CRs are doing, but for the core objects, it is in fact using duration expressed in seconds (see gracePeriodSeconds, timeoutSeconds or expirationSeconds).
OpenAPI seems to align also to the ISO format and XML Schema, although in Kubernetes they say:
duration: a duration string like \"22 ns\" as parsed by Golang time.ParseDuration or compatible with Scala duration format`
That said, I don't think the UI should display periods like PT1H30M but something more user-friendly. However, I do think the data sent over the wire should be using a standard instead of just using date parsing function that is only available in Golang. This way we would be forcing each client to build a Golang-alike duration parser, instead of just using the state-of-the-art existing date-handling dependency in the language the client uses.
If not... perhaps we should continue with the duration in seconds, so that the format is consistent across languages.
Anyway, no strong opinion here though. Let's see what others think. cc @absoludity @gfichtenholt
| TEST_TIMEOUT_MINUTES=${5:-"4"} | ||
| DEX_IP=${6:-"172.18.0.2"} | ||
| ADDITIONAL_CLUSTER_IP=${7:-"172.18.0.3"} | ||
| DOCKER_USERNAME=${8:-""} | ||
| DOCKER_PASSWORD=${9:-""} |
There was a problem hiding this comment.
Gret, thanks for changing it, but perhaps we have to sync with @castelblanque to ensure this change is not conflicting with his.
| if d, err := time.ParseDuration(duration); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Yes, I agree it differs from what the Flux/Carvel CRs are doing, but for the core objects, it is in fact using duration expressed in seconds (see gracePeriodSeconds, timeoutSeconds or expirationSeconds).
OpenAPI seems to align also to the ISO format and XML Schema, although in Kubernetes they say:
duration: a duration string like \"22 ns\" as parsed by Golang time.ParseDuration or compatible with Scala duration format`
That said, I don't think the UI should display periods like PT1H30M but something more user-friendly. However, I do think the data sent over the wire should be using a standard instead of just using date parsing function that is only available in Golang. This way we would be forcing each client to build a Golang-alike duration parser, instead of just using the state-of-the-art existing date-handling dependency in the language the client uses.
If not... perhaps we should continue with the duration in seconds, so that the format is consistent across languages.
Anyway, no strong opinion here though. Let's see what others think. cc @absoludity @gfichtenholt
antgamdia
left a comment
antgamdia
left a comment
There was a problem hiding this comment.
As per the offline discussion, LGTM. As you said, we are just passing the value and we don't expect the client to validate the value whatsoever.
|
@dlaloue-vmware, VMware has rejected your signed contributor license agreement. The change has already been merged, but it will be backed out by the project maintainers if the agreement is not resigned in a timely manner. Click here to resign the agreement. |
|
@dlaloue-vmware is a member of vmware-tanzu org. Removing cla-rejected label. |
|
@dlaloue-vmware, your company's legal contact has approved your signed contributor license agreement. It will also be reviewed by VMware, but the merge can proceed. |
4 participants
the current type for interval in repository (and installed package) was uint32, which is not very user friendly.
k8s uses a time+unit format to represent durations.
this change switches the type from uint32 to string.
it also provides 2 conversion methods ToDuration and FromDuration. The FromDuration also post-processes the string to remove 0 duration units (e.g. the k8s go code writes 24h as "24h0m0s", FromDuration strips the 0m and 0s to output as "24h")
The original goal was to use k8s.io apimachinery Duration directly in the proto (see issue #4869), but this solution had several issues and the intermediate solution is to at least support the string format, but do the conversion manually in the backend.