Skip to content

[JENKINS-64810] Add creation time boundaries to tags for discovery #1764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dodo920306 wants to merge 11 commits into
base: master
Choose a base branch
from
Open

[JENKINS-64810] Add creation time boundaries to tags for discovery #1764

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5b9acb0
Add creation time boundaries to tags for discovery
dodo920306 Jun 1, 2025
8a7de39
Initialize tag time boundaries with default values
dodo920306 Jan 23, 2026
77c1168
Handle errors when parsing tag day limits
dodo920306 Jan 23, 2026
d20c7ed
Add lower bonds to tag time boundaries
dodo920306 Jan 23, 2026
fcfb5f8
Merge remote-tracking branch 'upstream/master' into feature/ignore-ta…
dodo920306 Jan 23, 2026
df42f96
Log messages for skipping tags
dodo920306 Jan 23, 2026
2dea2d7
Fix tag age filtering to work correctly with annotated tags
dodo920306 Jan 23, 2026
ff5e2cd
Add a unit-test for AbstractGitSCMSource
dodo920306 Jan 23, 2026
fa788b3
Test nonnumeric values for TagDiscoveryTrait boundaries
dodo920306 Jan 23, 2026
1c59f53
Add help messages for tag time boundaries
dodo920306 Jan 26, 2026
1abea7a
Revert "Fix tag age filtering to work correctly with annotated tags"
dodo920306 Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/main/java/jenkins/plugins/git/AbstractGitSCMSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,9 @@
discoverBranches(repository, walk, request, remoteReferences);
}
if (context.wantTags()) {
discoverTags(repository, walk, request, remoteReferences);
discoverTags(repository, walk, request, remoteReferences,
context.getAtLeastTagCommitTimeMillis(),
context.getAtMostTagCommitTimeMillis());
}
if (context.wantOtherRefs()) {
discoverOtherRefs(repository, walk, request, remoteReferences,
Expand Down Expand Up @@ -772,7 +774,9 @@

private void discoverTags(final Repository repository,
final RevWalk walk, GitSCMSourceRequest request,
Map<String, ObjectId> remoteReferences)
Map<String, ObjectId> remoteReferences,
long atLeastMillis,
long atMostMillis)
throws IOException, InterruptedException {
listener.getLogger().println("Checking tags...");
walk.setRetainBody(false);
Expand All @@ -785,6 +789,21 @@
final String tagName = StringUtils.removeStart(ref.getKey(), Constants.R_TAGS);
RevCommit commit = walk.parseCommit(ref.getValue());
final long lastModified = TimeUnit.SECONDS.toMillis(commit.getCommitTime());

if (atLeastMillis >= 0L || atMostMillis >= 0L) {

Check warning on line 793 in src/main/java/jenkins/plugins/git/AbstractGitSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 793 is only partially covered, 2 branches are missing
if (atMostMillis >= 0L && atLeastMillis > atMostMillis) {
/* Invalid. It's impossible for any tag to satisfy this. */
continue;
}
Comment on lines +796 to +801

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. In the getTagCommitTimeLimitMillisFromDays method or the withAtLeast/withAtMost methods in GitSCMSourceContext
  2. 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.

Copilot uses AI. Check for mistakes.

@dodo920306 dodo920306 Jan 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary.

long tagAge = System.currentTimeMillis() - lastModified;
if (atMostMillis >= 0L && tagAge > atMostMillis) {
continue;
}
if (atLeastMillis >= 0L && tagAge < atLeastMillis) {
Comment thread
dodo920306 marked this conversation as resolved.
Outdated
continue;

Check warning on line 803 in src/main/java/jenkins/plugins/git/AbstractGitSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 794-803 are not covered by tests
}
}

if (request.process(new GitTagSCMHead(tagName, lastModified),
new SCMSourceRequest.IntermediateLambda<ObjectId>() {
@Nullable
Expand Down
34 changes: 34 additions & 0 deletions src/main/java/jenkins/plugins/git/GitSCMSourceContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.TimeUnit;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -95,6 +96,12 @@
@NonNull
private String remoteName = AbstractGitSCMSource.DEFAULT_REMOTE_NAME;

@CheckForNull
private long atLeastTagCommitTimeMillis;

@CheckForNull
private long atMostTagCommitTimeMillis;
Comment thread
dodo920306 marked this conversation as resolved.
Outdated
Comment thread
dodo920306 marked this conversation as resolved.
Outdated

/**
* Constructor.
*
Expand Down Expand Up @@ -192,6 +199,14 @@
return remoteName;
}

public final long getAtLeastTagCommitTimeMillis() {
return atLeastTagCommitTimeMillis;
}

public final long getAtMostTagCommitTimeMillis() {
return atMostTagCommitTimeMillis;
}

/**
* Adds a requirement for branch details to any {@link GitSCMSourceRequest} for this context.
*
Expand Down Expand Up @@ -358,6 +373,25 @@
return result;
}

private long getTagCommitTimeLimitMillisFromDays(String limitDays) {
long tagCommitTimeLimit = Long.parseLong(StringUtils.defaultIfBlank(limitDays, "-1"));
return tagCommitTimeLimit < 0 ? -1L : TimeUnit.DAYS.toMillis(tagCommitTimeLimit);

Check warning on line 378 in src/main/java/jenkins/plugins/git/GitSCMSourceContext.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 378 is only partially covered, one branch is missing
Comment thread
dodo920306 marked this conversation as resolved.
Outdated

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time conversion from days to milliseconds uses TimeUnit.DAYS.toMillis(tagCommitTimeLimit), which could theoretically overflow for very large day values. While unlikely in practice (would require values > ~106 million days), this could cause unexpected behavior.

The multiplication happens inside TimeUnit.toMillis which multiplies by 86,400,000. For values approaching Long.MAX_VALUE / 86,400,000 (~106,751,991 days), this could overflow.

Consider adding a reasonable upper bound check or using Math.multiplyExact for safer arithmetic. For example, limit the input to a reasonable maximum like 3650 days (10 years) with validation in the UI or conversion method.

Suggested change
private long getTagCommitTimeLimitMillisFromDays(String limitDays) {
long tagCommitTimeLimit = Long.parseLong(StringUtils.defaultIfBlank(limitDays, "-1"));
return tagCommitTimeLimit < 0 ? -1L : TimeUnit.DAYS.toMillis(tagCommitTimeLimit);
private static final long MAX_TAG_COMMIT_TIME_LIMIT_DAYS =
Long.MAX_VALUE / TimeUnit.DAYS.toMillis(1);
private long getTagCommitTimeLimitMillisFromDays(String limitDays) {
long tagCommitTimeLimit = Long.parseLong(StringUtils.defaultIfBlank(limitDays, "-1"));
if (tagCommitTimeLimit < 0) {
return -1L;
}
if (tagCommitTimeLimit > MAX_TAG_COMMIT_TIME_LIMIT_DAYS) {
throw new IllegalArgumentException("Tag commit time limit is too large: " + tagCommitTimeLimit + " days");
}
return TimeUnit.DAYS.toMillis(tagCommitTimeLimit);

Copilot uses AI. Check for mistakes.

@dodo920306 dodo920306 Jan 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I...am not quite sure about this?

}

@SuppressWarnings("unchecked")
@NonNull
public final C withAtLeastTagCommitTimeDays(String atLeastDays) {
this.atLeastTagCommitTimeMillis = getTagCommitTimeLimitMillisFromDays(atLeastDays);
return (C) this;
}

@SuppressWarnings("unchecked")
@NonNull
public final C withAtMostTagCommitTimeDays(String atMostDays) {
this.atMostTagCommitTimeMillis = getTagCommitTimeLimitMillisFromDays(atMostDays);
return (C) this;
}

/**
* {@inheritDoc}
*/
Expand Down
20 changes: 20 additions & 0 deletions src/main/java/jenkins/plugins/git/traits/TagDiscoveryTrait.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package jenkins.plugins.git.traits;

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import jenkins.plugins.git.GitSCMBuilder;
Expand Down Expand Up @@ -51,11 +52,20 @@
* @since 3.6.0
*/
public class TagDiscoveryTrait extends SCMSourceTrait {
private final String atLeastDays;
private final String atMostDays;

/**
* Constructor for stapler.
*/
@DataBoundConstructor
public TagDiscoveryTrait(@CheckForNull String atLeastDays, @CheckForNull String atMostDays) {
this.atLeastDays = atLeastDays;
this.atMostDays = atMostDays;
}

public TagDiscoveryTrait() {
this(null, null);
}

/**
Expand All @@ -66,6 +76,8 @@
GitSCMSourceContext<?,?> ctx = (GitSCMSourceContext<?, ?>) context;
ctx.wantTags(true);
ctx.withAuthority(new TagSCMHeadAuthority());
ctx.withAtLeastTagCommitTimeDays(atLeastDays)
.withAtMostTagCommitTimeDays(atMostDays);
}
Comment on lines +79 to 81

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Tags are correctly filtered when atLeastDays is set (tags newer than threshold are excluded)
  2. Tags are correctly filtered when atMostDays is set (tags older than threshold are excluded)
  3. Both parameters work together correctly
  4. Edge case: when atLeastDays > atMostDays (invalid configuration), no tags are discovered
  5. Null/empty values are handled correctly (no filtering applied)
  6. Invalid input (non-numeric strings) is handled gracefully

This is important to ensure the feature works as expected and to prevent regressions.

Copilot uses AI. Check for mistakes.

/**
Expand All @@ -76,6 +88,14 @@
return category instanceof TagSCMHeadCategory;
}

public String getAtLeastDays() {
return atLeastDays;
}

public String getAtMostDays() {
return atMostDays;

Check warning on line 96 in src/main/java/jenkins/plugins/git/traits/TagDiscoveryTrait.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 92-96 are not covered by tests
}

/**
* Our descriptor.
*/
Expand Down
6 changes: 6 additions & 0 deletions src/main/resources/jenkins/plugins/git/traits/TagDiscoveryTrait/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:c="/lib/credentials"
xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<f:entry field="atLeastDays" title="${%Ignore tags newer than}">
<f:number default=""/>
</f:entry>
<f:entry field="atMostDays" title="${%Ignore tags older than}">
Comment on lines +4 to +7

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
<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)}">

Copilot uses AI. Check for mistakes.
<f:number default="7"/>

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. What the fields control (filtering tags by age)
  2. How the age is calculated (from tag creation/commit time)
  3. What empty/blank values mean
  4. Examples of usage (e.g., "Enter 7 to ignore tags older than 7 days")
  5. 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.

Suggested change
<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>

Copilot uses AI. Check for mistakes.

@dodo920306 dodo920306 Jan 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no other traits that includes such help messages for their fields in this project.

@MarkEWaite MarkEWaite Jan 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dodo920306 dodo920306 Jan 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread
dodo920306 marked this conversation as resolved.
Outdated
Comment thread
dodo920306 marked this conversation as resolved.
Outdated
</f:entry>
Comment on lines +4 to +9

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If using the current labels, swap them so "Ignore tags older than" (atMostDays) comes first, then "Ignore tags newer than" (atLeastDays)
  2. 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.

Suggested change
<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>

Copilot uses AI. Check for mistakes.

@dodo920306 dodo920306 Jan 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt this.

</j:jelly>
Loading