Conversation
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Foreign commits detected in source branchThe source branch
This typically happens when the feature branch was accidentally based on
How to fix Create a new branch directly from Then open a new pull request from that branch. If this is a false positive If your branch is a legitimate backport and was previously merged into one |
fe41097 to
d65472e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 5 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.4 #2844 +/- ##
===================================================
- Coverage 74.88% 74.62% -0.27%
===================================================
Files 201 201
Lines 13761 13757 -4
===================================================
- Hits 10305 10266 -39
- Misses 3446 3481 +35
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
francoisferrand
left a comment
There was a problem hiding this comment.
I wonder if we should really fix this:
transitionOneDayEarlieris not really needed, since we can configure the system to transition after 0 day (expireOneDayEarlieris more useful, since the minimum delay for expiration is 1 day)- both flags are kind of deprecated,
timeProgressionFactorshould be used instead - while sound in principle (e.g. use
getTransitionTimestampconsistently to filter), I fear it may be complicated to insert cleanly in the code, without breaking abstractions levels...
I just saw this comment : I wonder, are these only used for our own testing, or do we have clients using them ? If we can confirm that these 2 expire/transitionOneDayEarlier are only used for easier testing and not by clients, then yeah imo we should leave or deprecate them more actively |
in zenko operator, they are called "xxxforTesting" : Probably worth to take a look and see if we can just remove that code and just keep time progression factor 🤔 |
SylvainSenechal
left a comment
There was a problem hiding this comment.
discussion, see comments
|
I reproduced both bugs on my artesca lab, and the fix ended up small, so I kept it. I'll check whether we can remove the deprecated flags and open a follow-up ticket if so. Edit: removing both env vars looks doable, so I dropped the |
d65472e to
ad0c265
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
ad0c265 to
155c9fd
Compare
francoisferrand
left a comment
There was a problem hiding this comment.
looks simpler indeed, but at the cost of duplication (2 "lifecycleDateTime") and CPU (computing the transition/expiration/current date multiple times, for each rule)... So still not sure it is worth the effort...
The v1 eligibility pre-filter and the noncurrent version transition apply compared raw rule days, ignoring transitionOneDayEarlier, while arsenal's getApplicableRules honors it. Eligible objects were skipped by the v1 pre-filter, and noncurrent versions were not transitioned early, also on the v2 task which reuses the same apply check. Compute transition times with LifecycleDateTime, as getApplicableRules does. Issue: BB-867
Transition times were compared with getCurrentDate(), which is shifted by expireOneDayEarlier, so that flag moved transitions one day earlier too: noncurrent version transitions on both v1 and v2 tasks, and the v1 eligibility pre-filter. With both flags set, the shifts stacked. Compare transition times with the real clock, as arsenal's getApplicableRules does. Issue: BB-867
155c9fd to
3a9910c
Compare
The v1 eligibility pre-filter and the noncurrent version transition apply compared raw rule days, ignoring transitionOneDayEarlier, so eligible objects were silently skipped while the current version apply stage honors the flag. Compute eligibility with LifecycleDateTime, as the apply stage does.
Issue: BB-867