[JENKINS-64810] Add creation time boundaries to tags for discovery#1764
[JENKINS-64810] Add creation time boundaries to tags for discovery#1764dodo920306 wants to merge 11 commits intojenkinsci:masterfrom
Conversation
Introduce upper and lower bounds for tag creation time in TagDiscoveryTrait, allowing AbstractGitSCMSource to determine whether a tag should be included based on its timestamp. Signed-off-by: dodo920306 <dodo920306@gmail.com>
|
Hello? |
Hi. I'm deeply involved in other topics currently. I don't expect to review this pull request for a month at least. You can use the incremental build of the pull request in your use so that you immediately get the benefit of your changes. When new changes merge to the master branch, update your branch with those changes and a new incremental build will be pushed to the Jenkins artifact repository. |
|
@MarkEWaite Is there any problem with this MR? |
I'm not available to review this pull request currently. Many other things are much higher priority for me. You're welcome to use the incremental build of the plugin yourself and report your results. You're welcome to encourage others to use the incremental build of the plugin and report their results. |
I found an immediate problem when I reviewed the names of the files that have been modified. The pull request includes no tests to show the problem and no tests to show that the problem is resolved. The git plugin is used in over 250,000 installations. I won't accept new functionality without very good automated test coverage. |
There was a problem hiding this comment.
Pull request overview
This PR adds time-based filtering capabilities to the Git plugin's TagDiscoveryTrait, allowing users to filter discovered tags by their creation time (commit time). This addresses JENKINS-64810 by providing a mechanism to limit the visibility of tag jobs based on age boundaries, complementing the existing build strategy filters that only prevent automatic builds.
Changes:
- Added
atLeastDaysandatMostDaysparameters toTagDiscoveryTraitto define minimum and maximum tag ages - Extended
GitSCMSourceContextwith methods to convert day values to milliseconds and store the time boundaries - Modified
AbstractGitSCMSource.discoverTags()to filter tags based on their age relative to current time
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| src/main/java/jenkins/plugins/git/traits/TagDiscoveryTrait.java | Added constructor parameters and fields for time boundaries, along with getter methods |
| src/main/java/jenkins/plugins/git/GitSCMSourceContext.java | Added fields to store time limits in milliseconds and conversion methods from days |
| src/main/java/jenkins/plugins/git/AbstractGitSCMSource.java | Added filtering logic in discoverTags method to skip tags outside the configured time window |
| src/main/resources/jenkins/plugins/git/traits/TagDiscoveryTrait/config.jelly | Added UI input fields for the time boundary parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <f:number default=""/> | ||
| </f:entry> | ||
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | ||
| <f:number default="7"/> |
There was a problem hiding this comment.
The new configuration fields "atLeastDays" and "atMostDays" lack help documentation. The config.jelly file adds these fields but there are no corresponding help-atLeastDays.html and help-atMostDays.html files to explain to users what these fields do.
These help files should explain:
- What the fields control (filtering tags by age)
- How the age is calculated (from tag creation/commit time)
- What empty/blank values mean
- Examples of usage (e.g., "Enter 7 to ignore tags older than 7 days")
- How these interact with each other (atMostDays should be greater than atLeastDays)
This is especially important since the UI labels "Ignore tags newer than" and "Ignore tags older than" might be confusing without additional context.
| <f:number default=""/> | |
| </f:entry> | |
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | |
| <f:number default="7"/> | |
| <f:number default=""/> | |
| <f:help> | |
| <p> | |
| Minimum age of tags (in days) to <strong>include</strong> when discovering tags. | |
| Tags newer than this value are ignored. | |
| </p> | |
| <p> | |
| Tag age is calculated from the tag creation / commit timestamp to the time the | |
| scan runs. | |
| </p> | |
| <p> | |
| Leave this field blank to <strong>not</strong> filter out tags based on a minimum age | |
| (no lower age bound). | |
| </p> | |
| <p> | |
| Examples: | |
| </p> | |
| <ul> | |
| <li><code>7</code> – ignore tags created in the last 7 days (only tags at least 7 days old are used).</li> | |
| <li><code>0</code> – do not ignore tags for being too new (equivalent to leaving it blank).</li> | |
| </ul> | |
| <p> | |
| When used together with <em>Ignore tags older than</em> (<code>atMostDays</code>), | |
| this value should be less than or equal to the <em>older than</em> value to define a valid age range. | |
| </p> | |
| </f:help> | |
| </f:entry> | |
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | |
| <f:number default="7"/> | |
| <f:help> | |
| <p> | |
| Maximum age of tags (in days) to <strong>include</strong> when discovering tags. | |
| Tags older than this value are ignored. | |
| </p> | |
| <p> | |
| Tag age is calculated from the tag creation / commit timestamp to the time the | |
| scan runs. | |
| </p> | |
| <p> | |
| Leave this field blank to <strong>not</strong> filter out tags based on a maximum age | |
| (no upper age bound). | |
| </p> | |
| <p> | |
| Examples: | |
| </p> | |
| <ul> | |
| <li><code>7</code> – ignore tags older than 7 days (only tags from the last 7 days are used).</li> | |
| <li><code>30</code> – ignore tags older than 30 days (only tags from the last 30 days are used).</li> | |
| </ul> | |
| <p> | |
| When used together with <em>Ignore tags newer than</em> (<code>atLeastDays</code>), | |
| this value should be greater than or equal to the <em>newer than</em> value so that the | |
| age range is consistent. | |
| </p> | |
| </f:help> |
There was a problem hiding this comment.
There is no other traits that includes such help messages for their fields in this project.
There was a problem hiding this comment.
There is no other traits that includes such help messages for their fields in this project.
GitHub Copilot suggested the wrong approach here. Please refer to the following example files for the correct approach to add online help to traits:
- src/main/resources/jenkins/plugins/git/traits/BranchDiscoveryTrait/help.html
- src/main/resources/jenkins/plugins/git/traits/DiscoverOtherRefsTrait/help-nameMapping.html
- src/main/resources/jenkins/plugins/git/traits/DiscoverOtherRefsTrait/help-ref.html
- src/main/resources/jenkins/plugins/git/traits/DiscoverOtherRefsTrait/help.html
- src/main/resources/jenkins/plugins/git/traits/RefSpecsSCMSourceTrait/RefSpecTemplate/help-value.html
- src/main/resources/jenkins/plugins/git/traits/TagDiscoveryTrait/help.html
There was a problem hiding this comment.
GitHub Copilot suggested the wrong approach here. Please refer to the following example files for the correct approach to add online help to traits:
* src/main/resources/jenkins/plugins/git/traits/BranchDiscoveryTrait/help.html * src/main/resources/jenkins/plugins/git/traits/DiscoverOtherRefsTrait/help-nameMapping.html * src/main/resources/jenkins/plugins/git/traits/DiscoverOtherRefsTrait/help-ref.html * src/main/resources/jenkins/plugins/git/traits/DiscoverOtherRefsTrait/help.html * src/main/resources/jenkins/plugins/git/traits/RefSpecsSCMSourceTrait/RefSpecTemplate/help-value.html * src/main/resources/jenkins/plugins/git/traits/TagDiscoveryTrait/help.html
OK.
| ctx.withAtLeastTagCommitTimeDays(atLeastDays) | ||
| .withAtMostTagCommitTimeDays(atMostDays); | ||
| } |
There was a problem hiding this comment.
The PR adds new functionality (time-based tag filtering) but mentions "None yet" for testing. The AbstractGitSCMSourceTest file already has comprehensive test coverage for TagDiscoveryTrait (as seen in tests like retrieveHeadsSupportsTagDiscovery_findTagsWithTagDiscoveryTrait), but there are no tests for the new time-based filtering parameters.
Tests should be added to verify:
- Tags are correctly filtered when atLeastDays is set (tags newer than threshold are excluded)
- Tags are correctly filtered when atMostDays is set (tags older than threshold are excluded)
- Both parameters work together correctly
- Edge case: when atLeastDays > atMostDays (invalid configuration), no tags are discovered
- Null/empty values are handled correctly (no filtering applied)
- Invalid input (non-numeric strings) is handled gracefully
This is important to ensure the feature works as expected and to prevent regressions.
| <f:entry field="atLeastDays" title="${%Ignore tags newer than}"> | ||
| <f:number default=""/> | ||
| </f:entry> | ||
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> |
There was a problem hiding this comment.
The UI field titles "Ignore tags newer than" and "Ignore tags older than" use double negatives that can be confusing. The field names are "atLeastDays" and "atMostDays" which refer to tag age requirements, but the labels use "ignore" which inverts the logic.
Consider more intuitive labels:
- For atLeastDays: "Minimum tag age (days)" or "Only discover tags at least this many days old"
- For atMostDays: "Maximum tag age (days)" or "Only discover tags at most this many days old"
Alternatively, use positive framing:
- For atLeastDays: "Discover tags older than (days)"
- For atMostDays: "Discover tags newer than (days)"
This would make the UI clearer and reduce the cognitive load on users trying to configure the right values.
| <f:entry field="atLeastDays" title="${%Ignore tags newer than}"> | |
| <f:number default=""/> | |
| </f:entry> | |
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | |
| <f:entry field="atLeastDays" title="${%Discover tags older than (days)}"> | |
| <f:number default=""/> | |
| </f:entry> | |
| <f:entry field="atMostDays" title="${%Discover tags newer than (days)}"> |
| if (atLeastMillis >= 0L || atMostMillis >= 0L) { | ||
| if (atMostMillis >= 0L && atLeastMillis > atMostMillis) { | ||
| /* Invalid. It's impossible for any tag to satisfy this. */ | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The filtering logic checks if atMostMillis >= 0L && atLeastMillis > atMostMillis and continues to skip the tag if this invalid configuration is detected. However, this check is performed inside the loop for every tag, which is inefficient.
Consider moving this validation earlier, such as:
- In the getTagCommitTimeLimitMillisFromDays method or the withAtLeast/withAtMost methods in GitSCMSourceContext
- At the beginning of the discoverTags method before the loop
This would provide earlier feedback to users about configuration errors and avoid redundant checks for every tag.
There was a problem hiding this comment.
I don't think this is necessary.
src/main/resources/jenkins/plugins/git/traits/TagDiscoveryTrait/config.jelly
Outdated
Show resolved
Hide resolved
| <f:entry field="atLeastDays" title="${%Ignore tags newer than}"> | ||
| <f:number default=""/> | ||
| </f:entry> | ||
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | ||
| <f:number default="7"/> | ||
| </f:entry> |
There was a problem hiding this comment.
The field order in the UI might be counterintuitive. The "atLeastDays" field (tags older than X days) appears before "atMostDays" field (tags newer than Y days). When users think about filtering by time range, they typically think in chronological order: "from X days ago to Y days ago" or in age order: "minimum age to maximum age".
Consider reordering the fields to be more intuitive:
- If using the current labels, swap them so "Ignore tags older than" (atMostDays) comes first, then "Ignore tags newer than" (atLeastDays)
- Or rename to "Minimum tag age" (atLeastDays) first, then "Maximum tag age" (atMostDays)
This would align better with user expectations when setting up a time range filter.
| <f:entry field="atLeastDays" title="${%Ignore tags newer than}"> | |
| <f:number default=""/> | |
| </f:entry> | |
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | |
| <f:number default="7"/> | |
| </f:entry> | |
| <f:entry field="atMostDays" title="${%Ignore tags older than}"> | |
| <f:number default="7"/> | |
| </f:entry> | |
| <f:entry field="atLeastDays" title="${%Ignore tags newer than}"> | |
| <f:number default=""/> | |
| </f:entry> |
src/main/resources/jenkins/plugins/git/traits/TagDiscoveryTrait/config.jelly
Outdated
Show resolved
Hide resolved
Tag time boundaries are primitive long which cannot be NULL, so the `@CheckForNull` annotations on them are useless and thus removed. Instead, default values `-1L` are applied to initialize them. Signed-off-by: dodo920306 <dodo920306@gmail.com>
If users input values that are not numbers to TagDiscoveryTrait day limits, an uncaught exception will be thrown for trying to parse them to long, which is dangerous and should be prevented. Signed-off-by: dodo920306 <dodo920306@gmail.com>
Add lower bonds "0" to tag time boundaries on UI. Moreover, for backward compatibility, the default value of `atMostDays` is removed. Signed-off-by: dodo920306 <dodo920306@gmail.com>
…gs-not-within-the-time-boundary
Signed-off-by: dodo920306 <dodo920306@gmail.com>
OK. Thanks for such prompt reply. To be honest, I am not in a hurry to use the new feature added in this PR. Yes, I do believe this is a long-overdue feature rather than merely a bug fix; my intention is simply to contribute to the project. I did not mean to rush you in any way—I just did not want this PR to be forgotten. Sorry for the interruption. Regarding the suggested changes automatically generated by Copilot, I have reviewed all of them. I may have missed or misunderstood some points, so if you believe any of the unaddressed suggestions are still necessary, please let me know. As for the tests, as mentioned in the PR description, I am not very confident about adding new code to the existing test suite. However, since you have requested it, I will give it a try. It would be even better if someone could provide some guidance. Thanks again! |
Use tag creation time instead of commit time when filtering tags by age. For annotated tags, extract the tagger timestamp; for lightweight tags, fall back to commit time. Both values are converted to milliseconds for consistent comparison. Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Added. Let's see the result. |
Signed-off-by: dodo920306 <dodo920306@gmail.com>
This reverts commit 2dea2d7. Signed-off-by: dodo920306 <dodo920306@gmail.com>
[JENKINS-64810] Multibranch pipeline still showing Git tags older than configured threshold
This PR does not address JENKINS-64810 in the way originally expected, because the issue stems from a misunderstanding of the help messages in the
basic-branch-build-strategies-plugin.The plugin currently describes the time-based tag filtering as:
and
The original issue incorrectly assumed that these settings should remove tag jobs outside the configured time range. However, what the
basic-branch-build-strategies-pluginactually implements is an extension ofjenkins.branch.BranchBuildStrategyfrom thebranch-api-plugin, which does not have the authority to remove jobs. It simply prevents automatic builds for tags outside the specified time window—which it already does correctly.While it might seem that this issue could be closed based on this clarification, I believe otherwise. After reaching out to the original reporter, Dirk Heinrichs, I found that—despite the misunderstanding of the underlying mechanism—their actual user need remains valid. As the reporter put it:
While this problem shouldn't be addressed by modifying the
basic-branch-build-strategies-plugin, the user still needs a way to visibly filter old or future tag jobs through the UI.After tracing through the codebase, I found that this functionality could be effectively implemented in the
git-plugin. In this PR, I introduce two new string parameters—atLeastDaysandatMostDays—to theTagDiscoveryTraitclass. These parameters mirror the ones from thebasic-branch-build-strategies-pluginand define the lower and upper bounds for the tag creation time.I then added corresponding logic to
GitSCMSourceContextto accept and propagate these values (converted intolong), and finally used them inAbstractGitSCMSourceinside thediscoverTagsfunction to filter tags based on age:This implementation and its corresponding UI (.jelly definitions) are heavily based on
TagBuildStrategyImplfrom thebasic-branch-build-strategies-plugin.If this PR is merged, users will be able to use
TagDiscoveryTraitto filter tag jobs by creation time, thereby fulfilling the practical need described in JENKINS-64810—even though the original interpretation of the issue was flawed.Please also let me know if there’s anything I should revise or improve. Thank you for your time and guidance!
Testing done
None yet.
I could use some help here, as I'm unsure how to add proper tests for this change in
AbstractGitSCMSourceTest.I've installed it on my local Jenkins instance, and it works well with my git tags.
Submitter checklist