Metadata TryGetTTL: adds time.ParseDuration support #3122
Conversation
Signed-off-by: robertojrojas <robertojrojas@gmail.com>
|
@robertojrojas and I chatted about this before. This is a good change - but as we are beyond code freeze this will be merged in 1.13. |
There was a problem hiding this comment.
I think this is a good change.
However, I'd like to ask that you rename ttlInSeconds to ttl after this change. ttlInSeconds should remain as alias, but the main property should be ttl (and if you can, please make the parser case-insensitive: this is one of the few properties that are still case-sensitive)
metadata/utils.go
Outdated
| // Overflow | ||
| duration = math.MaxInt64 | ||
| duration = time.Duration(valInt64) * time.Second | ||
| if duration < 0 { |
There was a problem hiding this comment.
Move this to outside of the if err != nil { block. Because the duration parsed by time.ParseDuration could be < 0 too
|
FYI: Throughout contrib for every component metadata of type Changing the TryGetTTL utility behavior here is consistent with the Duration behavior in Component Metadata parsing. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@robertojrojas are you still working on this? |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
…o metadata-get-ttl-fix Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
|
/ok-to-test |
Complete Build MatrixThe build status is currently not updated here. Please visit the action run below directly. Commit ref: dcc22f0 |
Components certification testCommit ref: dcc22f0 ✅ All certification tests passedAll tests have reported a successful status |
Components conformance testCommit ref: dcc22f0 ❌ Some conformance tests failedThese tests failed:
|
Signed-off-by: robertojrojas <robertojrojas@gmail.com> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Description
The metadata/utils.go.TryGetTTL doesn't appear to be parsing time.Duration correctly.
Providing a value like:
causes the function to return
Based on my understanding, this function should be able to parse both valid integers and time.Duration values.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: