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

[WIP] Log handling rewrite modifications #108

Closed
wants to merge 77 commits into from

Conversation

svanoort
Copy link
Member

@svanoort svanoort commented Oct 4, 2018

Builds on top of #27 -- do not merge yet (there's a direct PR to that branch open)

Unfortunately it is also necessary to <exclude> the official dependency GAs to avoid duplication,
which seems like a fundamental limitation of jitpack.io as applied to Maven.
jglick and others added 21 commits August 6, 2018 14:57
…nt which I think would need a patch to workflow-cps to handle properly.
…tep logs, even for steps running across the upgrade.

In this example, log is complete and the raw log is identical to what it would have been without an upgrade;
the first echo step and the sleep step use LogActionImpl and have complete *.log files;
the second echo step uses LogStorageAction and log-index records its range.
@jglick
Copy link
Member

jglick commented Oct 4, 2018

WorkflowRunRestartTest.lazyLoadExecution was one of the tests I noticed being flaky when I ran locally:

[WARNING] org.jenkinsci.plugins.workflow.job.WorkflowRunRestartTest.lazyLoadExecution(org.jenkinsci.plugins.workflow.job.WorkflowRunRestartTest)
[ERROR]   Run 1: WorkflowRunRestartTest.lambda$lazyLoadExecution$5:132 expected null, but was:<Owner[p/1:p #1]>
[ERROR]   Run 2: WorkflowRunRestartTest.lambda$lazyLoadExecution$5:132 expected null, but was:<Owner[p/1:p #1]>
[ERROR]   Run 3: WorkflowRunRestartTest.lambda$lazyLoadExecution$5:132 expected null, but was:<Owner[p/1:p #1]>
[ERROR]   Run 4: WorkflowRunRestartTest.lambda$lazyLoadExecution$5:132 expected null, but was:<Owner[p/1:p #1]>
[INFO]   Run 5: PASS

So probably just some sort of race condition.

@jglick
Copy link
Member

jglick commented Oct 4, 2018

FTR this is also jglick#3, but filed properly against the origin repo so that we get CI on it.

@svanoort
Copy link
Member Author

svanoort commented Oct 4, 2018

For whatever reason, what should be a trivial fix is not, and oddly the original does not trigger failures, so... I guess we roll with it.

@svanoort svanoort closed this Oct 4, 2018
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.

4 participants