-
Notifications
You must be signed in to change notification settings - Fork 268
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-5347] Added use commit times on files #116
base: master
Are you sure you want to change the base?
Conversation
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
Seems like the checkout failed (timeout) during the test |
The test failing (timeout) had nothing to do with the commit. |
@dvlemplek Can you re-base your pull request. The tests that are failing were fixed. |
Other tests failing, had nothing to do with the commit. |
* Gets the file that stores the revision. | ||
*/ | ||
@Deprecated | ||
public static File getRevisionFile(AbstractBuild build) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated to the pull request title.
IMO it should be reverted, because it breaks the backward compatibility.
The most of the pull requests looks good to me. If the backward compatibility issue gets fixed, I would vote for merging it |
You would have to tell me the JIRA ISSUE, since I don't have access to it. |
http://issues.jenkins-ci.org/browse/JENKINS-5347 |
Hi, I have the same need. Would be glad to see this pull request merged. Thank you |
@@ -56,5 +56,8 @@ THE SOFTWARE. | |||
<f:entry title="${%Filter changelog}" field="filterChangelog"> | |||
<f:checkbox /> | |||
</f:entry> | |||
<f:entry title="${%Set check out file dates to the 'last commit time'}" field="usingCommitTimes"> | |||
<f:checkbox /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabs in this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixed, but it's up o the plugin's maintainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now.
@dvlemplek |
@dvlemplek Do you plan to review @oleg-nenashev 's comments? |
@oleg-nenashev I thought I already did: ekesseler@c13f6f5 |
Please let me know if something is missing, I don't see it at the moment. |
FreeStyleProject p = createFreeStyleProject(); | ||
File repo = new CopyExisting(getClass().getResource("small.zip")).allocate(); | ||
SubversionSCM scm = new SubversionSCM(ModuleLocation.parse(new String[]{"file://" + repo.toURI().toURL().getPath()}, | ||
new String[]{"."},null,null), new UpdateUpdater(), null, "/z.*", "", "", "", "", false, false, null, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add spaces after the params.
@dvlemplek I'd like to make a smoke test before to merge this. I'll back here this night. |
@dvlemplek If you are working on JENKINS-5347, please accept the issue and update its status ( |
Should be all done now? |
👍 |
@@ -0,0 +1,8 @@ | |||
<div> | |||
If set Jenkins will set the file timestamps to the last commit time (of each file) when doing a checkout or an update. Otherwise Jenkins will set the current date as the timestamp of each file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence should be a paragraph, please use <p>
.
@dvlemplek As @oleg-nenashev said, the commits should be squashed into a single one |
08523ac
to
416efef
Compare
@dvlemplek We plan to cut a release (revision) at the end of this week. I hope to include your PR. |
How is the status on this? The build failed because of unrelated timeouts, can you trigger it again and include the pull request? |
@dvlemplek I'll try to review it as soon as possible. |
} | ||
|
||
/** | ||
* Test related to https://issues.jenkins-ci.org/browse/JENKINS-5347 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use the annotation @Issue
Merged upstream and fixed your comments. |
@dvlemplek Great! |
LGTM |
This is working as expected with a built version of the plugin on my end, thanks for the re-focus :-) |
Any hope of a merge anytime soon? |
Any news on this ? |
You do fantastic work with this plugin so I understand if we can get it in soon. If there is anyway, please consider it. We would love this fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, may have merge conflicts
Not accepting new features at this time unless obviously self-contained. (Also would need to use |
merge conflict |
http://issues.jenkins-ci.org/browse/JENKINS-5347
Applied your change requests from: https://github.com/jenkinsci/subversion-plugin/pull/64/files
Please approve and add this to the next version or give me feedback.
Thank you