[JENKINS-29034] - Modify filename to include date and time #38
Conversation
|
@reviewbybees Could you review this?, specially @stephenc @aheritier @ndeloof and others from Support Engineering Team. Read this to get more information https://github.com/cloudbees/cloudbees-support-plugin/pull/5 |
| // let the provider name it | ||
| filename = supportProvider.getName(); | ||
| filename = supportProvider.getName() + "_" + dateFormat.format(new Date()); |
There was a problem hiding this comment.
You need the date always, not just for those coming from the supportProvider. Do the date injection in the addHeader line not here
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
christ66
changed the title
| @@ -146,8 +152,10 @@ public void doDownload(StaplerRequest req, StaplerResponse rsp) throws ServletEx | |||
| if (supportPlugin != null) { | |||
| SupportProvider supportProvider = supportPlugin.getSupportProvider(); | |||
| if (supportProvider != null) { | |||
| SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd_HH.mm.ss"); | |||
There was a problem hiding this comment.
Creating a new SimpleDateFormat can be an expensive operation especially every time the doDownload link is clicked. I would recommend using a static final field since the format for the date/time never changes.
There was a problem hiding this comment.
but SimpleDateFormat is not thread safe, so general recommendation is to have one and clone it for use. Anyway, as this isn't an intensively used URL all this would be premature optimization imho
There was a problem hiding this comment.
Agree with @ndeloof
- This action is not heavily used, we can incur the cost of creating a new date format every time it is invoked. Users typically only access this link once or twice when they have an issue... and they are not performance sensitive compared to the time they will spend downloading the bundle
- The instance is not thread safe and is advertised as not thread safe, so if used form a static field you would need to add locking and synchronization which could potentially interfere with the issue the customer is encountering and prevent the generation of a bundle.
There was a problem hiding this comment.
@christ66 This part of the plugin is executed periodically and we use SimpleDateFormat.
|
@stephenc Could you review my last commit? Thanks. |
|
👍 |
1 similar comment
|
👍 |
[JENKINS-29034] - Modify filename to include date and time
|
@christ66 Thanks so much. |
https://issues.jenkins-ci.org/browse/JENKINS-29034