Skip to content

Make new timeout property usable in free-style jobs and add it to doc - #294

Merged
ogulcanaydogan merged 1 commit into
jenkinsci:masterfrom
HannesWell:timeout-for-freestyle-jobs
Aug 6, 2026
Merged

Make new timeout property usable in free-style jobs and add it to doc#294
ogulcanaydogan merged 1 commit into
jenkinsci:masterfrom
HannesWell:timeout-for-freestyle-jobs

Conversation

@HannesWell

@HannesWell HannesWell commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Add a timeout field to the SSHAgentBuildWrapper to make it usable in freestyle jobs, too.

Follow-up on

Personally I'm not using freestyle jobs, but for completes and feature parity, I think this would be good.

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@HannesWell
HannesWell requested a review from a team as a code owner July 30, 2026 22:11
@HannesWell

HannesWell commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

@ogulcanaydogan could you please review this?

@HannesWell HannesWell mentioned this pull request Jul 31, 2026
Merged
6 tasks
@HannesWell
HannesWell force-pushed the timeout-for-freestyle-jobs branch from 4c5592a to e2c5f41 Compare August 2, 2026 15:15
ogulcanaydogan
ogulcanaydogan reviewed Aug 3, 2026
View reviewed changes

@ogulcanaydogan ogulcanaydogan left a comment

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.

Thanks for picking this up, having the timeout on freestyle jobs too makes sense.

One thing before it actually works: the @DataBoundConstructor drops the timeout. It calls this(CredentialHolder.toIdList(credentialHolders), ignoreMissing), which routes to the deprecated (List<String>, boolean) overload, and that one fills in DEFAULT_TIMEOUT_MINUTES. So whatever the user types in the field never reaches this.timeout, it stays at 1. Passing it through fixes it:

this(CredentialHolder.toIdList(credentialHolders), ignoreMissing, timeout);

Smaller note: the new timeout field/getter is @since 405, but 405 already shipped on Jul 22 without it, so this one is the next release (@since FIXME like #286 did). The FIXME to 405 changes on the ExecRemoteAgent side are right, that code did land in 405.

@HannesWell
HannesWell force-pushed the timeout-for-freestyle-jobs branch from e2c5f41 to a866f51 Compare August 3, 2026 16:03
@HannesWell

HannesWell commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

One thing before it actually works: the @DataBoundConstructor drops the timeout.

Oh yes, I missed that. Thanks for pointing out. Fixed it (as suggested).

Smaller note: the new timeout field/getter is @since 405, but 405 already shipped on Jul 22 without it, so this one is the next release (@since FIXME like #286 did). The FIXME to 405 changes on the ExecRemoteAgent side are right, that code did land in 405.

Yes. I wanted to be a bit 'lazy' and avoid subsequent changes by using the version where the timeout feature generally became available. But changed it as you suggested.
And added javadoc for the new parameters in the constructors.

ogulcanaydogan
ogulcanaydogan approved these changes Aug 4, 2026
View reviewed changes

@ogulcanaydogan ogulcanaydogan left a comment

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.

Nice, thanks for the quick fix. The DataBoundConstructor threads the timeout through now, and the @since split reads right (FIXME for the freestyle field, 405 for the ExecRemoteAgent bits that already shipped). Good call extending the timeout to freestyle jobs too, that was a real gap. Approving, thanks!

@HannesWell

HannesWell commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Great, thanks for your reviews.

Can this and my other ready/approved PR thru be submitted?
Thanks in advance.

@ogulcanaydogan

ogulcanaydogan commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Happy to land all three! Since they overlap in ExecRemoteAgent, I will merge them one at a time and each following one will need a quick rebase. Starting with #295 (the refactor). Once it is in, could you rebase #294 on top? Then after #294 lands, #296 becomes just the executable delta. That keeps your three as distinct changes. Thanks for all the work on these.

@HannesWell
Make new timeout property usable in free-style jobs and add it to doc
6dfde48 
Add a timeout field to the SSHAgentBuildWrapper to make it usable in
freestyle jobs, too.
@HannesWell
HannesWell force-pushed the timeout-for-freestyle-jobs branch from a866f51 to 6dfde48 Compare August 6, 2026 20:29
@HannesWell

HannesWell commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Once it is in, could you rebase #294 on top? Then after #294 lands, #296 becomes just the executable delta.

Sounds good. Just rebased this one. I had a rebase butten in the GH UI.
The last of the three seems to need a manual rebase. Will do that later this evening.

@ogulcanaydogan
ogulcanaydogan merged commit aaa827a into jenkinsci:master Aug 6, 2026
15 checks passed
@HannesWell
HannesWell deleted the timeout-for-freestyle-jobs branch August 6, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogulcanaydogan ogulcanaydogan ogulcanaydogan approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@HannesWell @ogulcanaydogan