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

Jump to bottom

[FIXED JENKINS-10629] archiving #1670

Merged
merged 5 commits into from Aug 26, 2015
Merged

[FIXED JENKINS-10629] archiving #1670

merged 5 commits into from Aug 26, 2015

Conversation

KostyaSha
Copy link
Member

Fixed @oleg-nenashev bug in feature that was reverted after 1.610.

Review on Reviewable

@KostyaSha KostyaSha closed this Apr 23, 2015
@KostyaSha KostyaSha reopened this Apr 23, 2015
@KostyaSha KostyaSha changed the title WIP [FIXED JENKINS-10629] archiving [FIXED JENKINS-10629] archiving Apr 25, 2015
@oleg-nenashev
Copy link
Member

@KostyaSha
Thanks for your efforts on the issue troubleshooting and fixing. Probably, a cherry-pick can be preferable in order to have a native reference to a commit you want to restore.

I propose to merge the change to jenkins-1.612, because we're not sure that all regressions have been fixed. I also committed to provide more tests to achieve a better coverage, so it makes sense to wait for a while.

@KostyaSha
Copy link
Member Author

Revert of revert is the exact history action, branching commits with cherry-picking is bad idea.

oleg-nenashev referenced this pull request May 2, 2015
@oleg-nenashev
Merge pull request #1667 from oleg-nenashev/JENKINS-10629-FIX
ca21c65 
[JENKINS-28013,JENKINS-28012] - Revert changes for JENKINS-10629
@oleg-nenashev oleg-nenashev changed the title [FIXED JENKINS-10629] archiving [WiP] [FIXED JENKINS-10629] archiving May 2, 2015
@oleg-nenashev oleg-nenashev self-assigned this May 2, 2015
@oleg-nenashev
Copy link
Member

It's my technical debt to provide unit tests for this change

@daniel-beck
Copy link
Member

JENKINS-28124 may be related: another issue with archiving (zip in this case) possibly introduced by the original commons-compress change and fixed by reverting.

@daniel-beck daniel-beck added the work-in-progress The PR is under active development, not ready to the final review label Jun 11, 2015
@oleg-nenashev
Copy link
Member

I have not developed the tests yet, but the patch after the @KostyaSha's adjustment works well on my home server for two months. Probably, it's enough to make another attempt.

@KostyaSha KostyaSha mentioned this pull request Aug 20, 2015
Closed
@KostyaSha KostyaSha changed the title [WiP] [FIXED JENKINS-10629] archiving [FIXED JENKINS-10629] archiving Aug 20, 2015
@oleg-nenashev
Copy link
Member

@KostyaSha @daniel-beck
Do you agree with merging this PR without additional tests ? The new change just fixes the flushing and it is confirmed to work on archiving cases, which had a regression in the first fix.

@KostyaSha
Copy link
Member Author

@oleg-nenashev of course, but i thought you already has tests that just need to be cherry-picked. Or point me on master-slave example and i will wrote a test case

@daniel-beck
Copy link
Member

@oleg-nenashev Also not opposed if the tests are too complicated.

@KostyaSha
Copy link
Member Author

@daniel-beck i found how simply wrote a test, working on it

@KostyaSha
Copy link
Member Author

Can't reproduce issue using jnlp slave, maybe issues tied only to ssh slaves.

@KostyaSha
Copy link
Member Author

Reproduced! 1 byte less or more and test wouldn't work, initially tested on top of 1.610 that fails test.

@KostyaSha
Copy link
Member Author 

jenkins.widgets.BuildListTableTest.linksFromFolders

Failing for the past 1 build (Since Unstable#3044 )
Took 9 sec.
Error Message

elementName=[a] attributeName=[<text>] attributeValue=[d » d2 » p]
Stacktrace

com.gargoylesoftware.htmlunit.ElementNotFoundException: elementName=[a] attributeName=[<text>] attributeValue=[d » d2 » p]
    at com.gargoylesoftware.htmlunit.html.HtmlPage.getAnchorByText(HtmlPage.java:569)
    at jenkins.widgets.BuildListTableTest.linksFromFolders(BuildListTableTest.java:60)

Unrelated...

KostyaSha and others added 4 commits August 25, 2015 01:09
@KostyaSha
Revert "Revert "[JENKINS-10629] - Migrate the Tar archives handling c…
03bd595 
…ode to commons-compress""

This reverts commit 8f1280a.
@KostyaSha
Revert "Revert "FIXED JENKINS-10629] - Enable BigNumber mode to suppo…
2ef2921 
…rt archiving of files with size >8Gb""

This reverts commit ee57300.
@KostyaSha
[FIXED JENKINS-10629] Unbroke stream with new tar implementation.
3e187a0 
Causes bytes lost and truncated tar archive.
TarBuffer not used in TarArchiveOutputStream.
@KostyaSha
Remove hardcode in tar test
bfa6d8b
@KostyaSha
Copy link
Member Author

Fixuped testcase into fix, re-triggering pr.

@KostyaSha
Copy link
Member Author

@oleg-nenashev compress-commons 1.10 released ;) Will try update when current build will be green.

@KostyaSha
Copy link
Member Author

Done!

@oleg-nenashev
Copy link
Member

👍

@KostyaSha
Copy link
Member Author

@oleg-nenashev everybody refuse merging it because you are in assignee.

@jglick jglick merged commit 8e97fd1 into jenkinsci:master Aug 26, 2015
jglick added a commit that referenced this pull request Aug 26, 2015
@jglick
[JENKINS-10629] Merging #1670.
b5b3779
@jglick jglick removed the work-in-progress The PR is under active development, not ready to the final review label Aug 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oleg-nenashev oleg-nenashev

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@KostyaSha @oleg-nenashev @daniel-beck @jglick