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.

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

[JENKINS-23571] Add configuration option for cache directory location #91

Merged
merged 2 commits into from Jan 13, 2017

Conversation

Projects
None yet
2 participants
@mryan43
Copy link
Contributor

commented Nov 25, 2016

JENKINS-23571

This PR adds a new optional configuration option for "mercurial installations".

This new options allows users to customize the location of cached mercurial repositories on the jenkins master node when using repository caching. The default value remains unchanged ($JENKINS_HOME/hgcache)

This feature is useful for people who want to place this cache outside of $JENKINS_HOME.


This change is Reviewable

@jglick

jglick approved these changes Dec 5, 2016

}

private static final Map<String, Cache> CACHES = new HashMap<String, Cache>();

public synchronized static @NonNull Cache fromURL(String remote, StandardUsernameCredentials credentials) {
String h = hashSource(remote, credentials);
public synchronized static @NonNull Cache fromURL(String remote, StandardUsernameCredentials credentials, String masterCacheRoot) {

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

@CheckForNull

@@ -37,6 +38,8 @@
*/
private final String remote;

private final String masterCacheRoot;

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

@CheckForNull


FilePath masterCaches = null;
if (masterCacheRoot != null){
masterCaches = new FilePath(master.getChannel(), masterCacheRoot);

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

new File(masterCacheRoot) suffices.

This comment has been minimized.

Copy link
@mryan43

mryan43 Dec 7, 2016

Author Contributor

How should I deal with the following
FilePath masterCache = masterCaches.child(hash);
which is 9 lines lower ?

This comment has been minimized.

Copy link
@jglick

jglick Jan 4, 2017

Member

Sorry, I meant

masterCaches = new FilePath(masterCacheRoot);
@@ -119,7 +129,7 @@ private synchronized ReentrantLock getLockForSlaveNode(String node) {
try {

// Lock the block used to verify we end up having a cloned repo in the master,
// whether if it was previously cloned in a different build or if it's
// whether if it was previously cloned in a different build or if it's

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

Please avoid gratuitous diff hunks, including whitespace-only changes. Configure your editor to not edit lines of a file you did not actually type in.

@@ -241,12 +251,19 @@ private synchronized ReentrantLock getLockForSlaveNode(String node) {
/**
* Hash a URL into a string that only contains characters that are safe as directory names.
*/
static String hashSource(String source, StandardUsernameCredentials credentials) {
static String hashSource(String source, StandardUsernameCredentials credentials, String masterCacheRoot) {

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

@CheckForNull

digestible += '#' + credentials.getId();
}
if (masterCacheRoot != null){
digestible += "#" + masterCacheRoot.replaceAll(File.pathSeparator, "_");

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

You do not need to replace the pathSeparator here—this is just input to a digest, not part of the final directory name.

This comment has been minimized.

Copy link
@mryan43

mryan43 Dec 7, 2016

Author Contributor

The intent here was to avoid environment specific data in the digest hash so that it would be the same on windows and linux for example.

This comment has been minimized.

Copy link
@jglick

jglick Jan 4, 2017

Member

OK…irrelevant since a given master is either running on Windows or it is running on Unix-ish, and that is very unlikely to change between restarts. But harmless I suppose.

}

@DataBoundConstructor public MercurialInstallation(String name, String home, String executable, boolean debug, boolean useCaches, boolean useSharing, String config, List<? extends ToolProperty<?>> properties) {
/** for backwards compatibility */

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

Could probably switch to a @DataBoundConstructor with just name and home and properties parameters, and use @DataBoundSetter for all the optional stuff. But cannot remember trying it in the context of a ToolInstallation.

This comment has been minimized.

Copy link
@mryan43

mryan43 Dec 7, 2016

Author Contributor

I mimicked how it was done when the "config" option was added. According to your suggestion, I suppose the constructor which doesn't take a "config" should also be removed, which would be an unrelated change to the feature I'm trying to add.

j.jenkins
.getDescriptorByType(MercurialInstallation.DescriptorImpl.class)
.setInstallations(new MercurialInstallation(CACHING_INSTALLATION, "", "hg",
false, true, Paths.get("target", "test-data", "custom-cache-dir").toString(), false, "",

This comment has been minimized.

Copy link
@jglick

jglick Dec 5, 2016

Member

Use tmp.newFolder().getAbsolutePath() instead.

This comment has been minimized.

Copy link
@mryan43

mryan43 Dec 7, 2016

Author Contributor

I still need "custom-cache-dir" in the path to be able to assert that the new configuration option was really used as target path for cloning. I'll change it to new File(tmp.newFolder(),"custom-cache-dir").getAbsolutePath() if that's ok for you.

Implement feedback from Jesse's first review of PR 91 on github : add @…
…checkfornull, rollback unrelated whitespace changes, change test temporary folder to use a TemporaryFolder instead of a relative path
@mryan43

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

I have pushed fixes regarding your comments, do you want me to squash it with the initial commit ?

@jglick

This comment has been minimized.

Copy link
Member

commented Dec 16, 2016

No squashing please.

@jglick jglick self-assigned this Dec 16, 2016

@jglick

jglick approved these changes Jan 4, 2017


FilePath masterCaches = null;
if (masterCacheRoot != null){
masterCaches = new FilePath(master.getChannel(), masterCacheRoot);

This comment has been minimized.

Copy link
@jglick

jglick Jan 4, 2017

Member

Sorry, I meant

masterCaches = new FilePath(masterCacheRoot);
digestible += '#' + credentials.getId();
}
if (masterCacheRoot != null){
digestible += "#" + masterCacheRoot.replaceAll(File.pathSeparator, "_");

This comment has been minimized.

Copy link
@jglick

jglick Jan 4, 2017

Member

OK…irrelevant since a given master is either running on Windows or it is running on Unix-ish, and that is very unlikely to change between restarts. But harmless I suppose.

@jglick jglick merged commit 4a03ff9 into jenkinsci:master Jan 13, 2017

1 check passed

Jenkins This pull request looks good
Details

jglick added a commit that referenced this pull request Jan 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.