argo-cd icon indicating copy to clipboard operation
argo-cd copied to clipboard

fix: Enhance time duration parsing for cache durations

Open pdrastil opened this issue 2 years ago • 9 comments

Fixes https://github.com/argoproj/argo-cd/issues/9328

Checklist:

  • [x] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • [x] The title of the PR states what changed and the related issues number (used for the release note).
  • [x] I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • [ ] Does this PR require documentation updates?
  • [x] I've updated documentation as required by this PR.
  • [ ] Optional. My organization is added to USERS.md.
  • [x] I have signed off all my commits as required by DCO
  • [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • [x] My build is green (troubleshooting builds).

pdrastil avatar May 13 '22 16:05 pdrastil

Codecov Report

Merging #9405 (b8315b0) into master (470176b) will increase coverage by 0.01%. The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master    #9405      +/-   ##
==========================================
+ Coverage   45.92%   45.93%   +0.01%     
==========================================
  Files         227      227              
  Lines       27403    27417      +14     
==========================================
+ Hits        12584    12595      +11     
- Misses      13110    13112       +2     
- Partials     1709     1710       +1     
Impacted Files Coverage Δ
util/env/env.go 85.54% <76.47%> (-1.42%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 470176b...b8315b0. Read the comment docs.

codecov[bot] avatar May 13 '22 16:05 codecov[bot]

@crenshaw-dev I'm probably not seeing why this could be breaking. Can you please explain? If you provide just single digit number it still works as before (currently: 90s = time.ParseDuration would allow to use 90s or 1m30s).

pdrastil avatar May 27 '22 15:05 pdrastil

@pdrastil that's true... the existing parser adds support for d, which I don't think is supported by time.ParseDuration.

crenshaw-dev avatar May 27 '22 17:05 crenshaw-dev

Although we do use time.ParseDuration for a lot of other stuff, so I definitely think it's worth switching eventually. Just maybe by toggling on the presence of a d to avoid making a hard break?

crenshaw-dev avatar May 27 '22 17:05 crenshaw-dev

Thanks for the explanation - from documentation I've seen the d is not present because it could cause ambiguity. Ref: https://github.com/golang/go/issues/17767

Kinda elegant solution would be:

  • Regex capture of ([0-9]+)d from original string
  • Parse the rest with time util
  • Add 24h * number of days to result

EDIT: Question is if such change should be done in the pkg or in this codebase because it seems that this was the only place that used that logic.

pdrastil avatar May 27 '22 18:05 pdrastil

I like that solution!

I see four places in the code using that utility function. To be honest, I'd rather copy/paste the function into Argo CD and modify it than editing it in the source package. I don't see the value of abstracting that function out.

crenshaw-dev avatar May 27 '22 21:05 crenshaw-dev

@crenshaw-dev Enhanced as proposed. As a bonus fractions now works as well like 1.5d

pdrastil avatar May 29 '22 08:05 pdrastil

@crenshaw-dev Hi, any chance you can take a look again?

pdrastil avatar Jun 07 '22 06:06 pdrastil

@crenshaw-dev Hi - this is here for a long time - any chance you can look again?

pdrastil avatar Jul 29 '22 17:07 pdrastil

In general LGTM (once the merge conflicts have been fixed)

blakepettersson avatar Sep 22 '23 15:09 blakepettersson