Conversation
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| (t.NoncurrentDays !== undefined && daysSinceInitiated >= t.NoncurrentDays)); | ||
| return rule.NoncurrentVersionTransitions.some(t => { | ||
| const transitionTime = this._lifecycleDateTime | ||
| .getNCVTransitionTimestamp(t, entity.LastModified); |
There was a problem hiding this comment.
entity.LastModified should be staleDate here. For noncurrent versions, NoncurrentDays counts from when the version became noncurrent (staleDate), not from its creation date (LastModified). The apply stage at line 1392 correctly passes staleDate to the same method. Using LastModified causes a mismatch between eligibility filtering and actual rule application when the two dates differ.
| .getNCVTransitionTimestamp(t, entity.LastModified); | |
| .getNCVTransitionTimestamp(t, staleDate); |
| }, | ||
| ]; | ||
| nonCurrentVersion.LastModified = new Date(Date.now() - HOUR).toISOString(); | ||
| nonCurrentVersion.staleDate = new Date(Date.now() - HOUR).toISOString(); |
There was a problem hiding this comment.
Setting both LastModified and staleDate to the same value masks the bug above — the test passes regardless of which date getNCVTransitionTimestamp receives. Use different values to actually verify the correct date is used, e.g. set LastModified far in the past and staleDate to one hour ago.
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 |
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
fe41097 to
d65472e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 6 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.4 #2844 +/- ##
===================================================
- Coverage 74.88% 74.63% -0.26%
===================================================
Files 201 201
Lines 13761 13759 -2
===================================================
- Hits 10305 10269 -36
- Misses 3446 3480 +34
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
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...
| * @return {boolean} true if rule applies - false otherwise. | ||
| */ | ||
| _isRuleApplying(rule, daysSinceInitiated, currentDate) { | ||
| _isRuleApplying(rule, daysSinceInitiated, currentDate, lastModified) { |
There was a problem hiding this comment.
it seems weird to add lastModified here: since daysSinceInitiated is already computed from lastModified...
looking at the code in LifecycleDateTime.findDaysSince(), it seems the issue is really that findDaysSince() uses expireOneDayEarlier systematically: so that flag affects both transition & expiration, instead of only expiration and the other flag affecting transitions...
There was a problem hiding this comment.
Looking at the code structure of isRuleApplying : Before the change, the code was similar for both expiration and transition. Now transition uses Arsenal helper while expiration manually checks the date.
It's a bit annoying to create another ticket in arsenal, but maybe we should have a getExpirationTimestamp helper in arsenal ? and this way, this fuction isRuleApplying will probably only need 2 parameters rule + lastModified I guess
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
SylvainSenechal
left a comment
There was a problem hiding this comment.
discussion, see comments
4 participants
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