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-5347] Added use commit times on files #116

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ekesseler
Copy link

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

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@ekesseler
Copy link
Author

Seems like the checkout failed (timeout) during the test

@ekesseler
Copy link
Author

The test failing (timeout) had nothing to do with the commit.

@christ66
Copy link
Member

christ66 commented Jul 2, 2015

@dvlemplek Can you re-base your pull request. The tests that are failing were fixed.

@ekesseler
Copy link
Author

Other tests failing, had nothing to do with the commit.

* Gets the file that stores the revision.
*/
@Deprecated
public static File getRevisionFile(AbstractBuild build) {
Copy link
Member

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.

@oleg-nenashev
Copy link
Member

The most of the pull requests looks good to me. If the backward compatibility issue gets fixed, I would vote for merging it

@ekesseler
Copy link
Author

You would have to tell me the JIRA ISSUE, since I don't have access to it.

@oleg-nenashev
Copy link
Member

http://issues.jenkins-ci.org/browse/JENKINS-5347
The entire JENKINS project in JIRA is public, hence you should have an access

@tomap
Copy link

tomap commented Jul 9, 2015

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 />
Copy link
Member

Choose a reason for hiding this comment

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

tabs in this line

Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed now.

@oleg-nenashev
Copy link
Member

@dvlemplek
👍 for the PR when a minor comment gets fixed.
BTW, the commits should be squashed into a single one

@recena
Copy link
Contributor

recena commented Aug 6, 2015

@dvlemplek Do you plan to review @oleg-nenashev 's comments?

@ekesseler
Copy link
Author

@oleg-nenashev I thought I already did: ekesseler@c13f6f5

@ekesseler
Copy link
Author

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);
Copy link
Contributor

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.

@recena
Copy link
Contributor

recena commented Aug 13, 2015

@dvlemplek I'd like to make a smoke test before to merge this. I'll back here this night.

@recena recena changed the title JENKINS-5347 Fixed - Added use commit times on files [JENKINS-5347] Added use commit times on files Aug 13, 2015
@recena
Copy link
Contributor

recena commented Aug 13, 2015

@dvlemplek If you are working on JENKINS-5347, please accept the issue and update its status (in progress).

@ekesseler
Copy link
Author

Should be all done now?

@oleg-nenashev
Copy link
Member

👍

@@ -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.
Copy link
Contributor

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

@recena
Copy link
Contributor

recena commented Aug 17, 2015

@dvlemplek As @oleg-nenashev said, the commits should be squashed into a single one

@ekesseler ekesseler force-pushed the add-commit-times-support branch from 08523ac to 416efef Compare August 17, 2015 10:30
@recena
Copy link
Contributor

recena commented Aug 17, 2015

@dvlemplek We plan to cut a release (revision) at the end of this week. I hope to include your PR.

@ekesseler
Copy link
Author

How is the status on this? The build failed because of unrelated timeouts, can you trigger it again and include the pull request?

@recena
Copy link
Contributor

recena commented Jan 8, 2016

@dvlemplek I'll try to review it as soon as possible.

}

/**
* Test related to https://issues.jenkins-ci.org/browse/JENKINS-5347
Copy link
Contributor

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

@ekesseler
Copy link
Author

Merged upstream and fixed your comments.

@recena
Copy link
Contributor

recena commented Jan 11, 2016

@dvlemplek Great!

@christ66
Copy link
Member

LGTM

@unitychrism
Copy link

This is working as expected with a built version of the plugin on my end, thanks for the re-focus :-)

@unitychrism
Copy link

Any hope of a merge anytime soon?

@kennetn
Copy link

kennetn commented Aug 22, 2016

Any news on this ?

@DaveNoth
Copy link

DaveNoth commented Sep 8, 2016

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.

Copy link
Member

@oleg-nenashev oleg-nenashev left a 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

@jglick
Copy link
Member

jglick commented Jun 16, 2017

Not accepting new features at this time unless obviously self-contained. (Also would need to use @DataBoundSetter.)

@oleg-nenashev
Copy link
Member

merge conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants