Skip to content
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

Jump to bottom

[JENKINS-29034] - Modify filename to include date and time #38

Merged
merged 2 commits into from Jun 25, 2015
Merged

[JENKINS-29034] - Modify filename to include date and time #38

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51551c8
Related to [JENKINS-29034] - Modify filename to include date and time
Jun 23, 2015
0cfb539
The date is injected when we define the HTTP header
Jun 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/main/java/com/cloudbees/jenkins/support/SupportAction.java
View file
Open in desktop
Expand Up @@ -26,12 +26,14 @@

import com.cloudbees.jenkins.support.api.Component;
import com.cloudbees.jenkins.support.api.SupportProvider;

import hudson.Extension;
import hudson.model.RootAction;
import hudson.security.ACL;
import hudson.security.Permission;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;

import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.jvnet.localizer.Localizable;
Expand All @@ -42,12 +44,16 @@
import javax.servlet.ServletException;
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletResponse;

import java.io.IOException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.util.TimeZone;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand Down Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @ndeloof

  1. 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
  2. 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.

Copy link
Author

Choose a reason for hiding this comment

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

@christ66 This part of the plugin is executed periodically and we use SimpleDateFormat.

dateFormat.setTimeZone(TimeZone.getTimeZone("GMT"));
// let the provider name it
filename = supportProvider.getName();
filename = supportProvider.getName() + "_" + dateFormat.format(new Date());
Copy link
Member

Choose a reason for hiding this comment

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

You need the date always, not just for those coming from the supportProvider. Do the date injection in the addHeader line not here

}
}
rsp.addHeader("Content-Disposition", "inline; filename=" + filename + ".zip;");
Expand Down