Show an error if creating the upload archive fails

[chrismathis]

e.g if there are duplicate jars. In my case feature and plug-in had the same name

[chrismathis]

After some additional experiments I have to say I like logging only the exception message better. Because the method is called for each file uploaded you might end up with lots of similar stack traces otherwise.

[nfalco79]

Could you provide a single squash commit?

[ctron]

@chrismathis sorry for the confusion I caused. Please revert to the original way of logging this. I was under the impression that this would be logged to stdout.

[chrismathis]

@ctron the latest change is motivated by the fact that the method is called for each file (which might be many). This might lead the very many similar Stacktraces in the console. I recognized that when throwing a new Exception in the upload try-catch to test out the log entries.

Do you prefer the (potentially) many Stacktraces over single line messages?

[ctron]

I prefer not the have any errors 😁

I am not sure about this. What you could do is to actually show the full stack trace when the "failed" flag is still false and the compressed message afterwards. That way you would have the full stack trace, but only once.

What do you think?

[chrismathis]

That sounds good - i'll do that.

[nfalco79]

At this point, why do not you check the failed flag at the beginning of the method instead of creating the entries since the work that would be lost? So I would expect:

Failed to upload file1 to archive to upload: Space left on device
file2 was skipped due previous error (<- or log nothing)
file3 was skipped due previous error (<- or log nothing)
...

[ctron]

So you mean not to even try? Sounds reasonable to me.

[chrismathis]

@nfalco79 you are right. It does not make much sense to write the files to the archive when complete() (which does the actual upload) will do nothing if this.failed.

[chrismathis]

Should IOExceptions actually be ignored if failsAsUpload is false? That does not work for V2 at the moment (at least in pipeline jobs). They fail the build.

[nfalco79]

I think the UploadV2 simply miss to handle IOException. I think it should be always catch and log it (but I prefer without whole tracktrace in the build log) Only @ctron could say if the original idea was continue the upload of the following files or stop it. V3 acts like a transaction where all files are uploaded or none, V2 no. The Upload implementation should be aware of failure strategy. Is the recorder that based on user choice do the work

[nfalco79]

I have to correct my previous comment (In these days I'm writing aware from home and PC, so pardon my errors). The interface Uploader have the method upload that returns void and declare to throws IOException. So in the implementation if any IO error occur could throwed. It is the callable UploadFiles that should catch and handle the exception (and maybe log it). So about the V3 I can say that the original implementation simply miss a throw e after set the failed flag.

[ctron]

No worries ;-)

To be honest, I have. I idea what I had in mind :-D

I guess throwing it the same way would be a reasonable way to do it.

[chrismathis]

I've refactored the Uploaders to throw Exceptions instead of setting a flag. The IOExceptions are caught in UploadFiles and returned by invoke. If they were thrown Jenkins would add: remote file operation failed: " + remote + " at " + channel + ": ".

Reading fields of FileCallables does not seem to work if they are executed on a slave. They do not seem to get serialized back. So isFailed and isEmptyArchive always returned false.

Jobs will now only fail if failsAsUpload is true or allowEmptyArchive is false when the respective errors occur. (the empty archive behavior was inverted)

[nfalco79]

The FileCallable serialise back just only the returned value and not the entire instance. Since the UploadFiles must collect the artifacts to construct the BuildData it should return Map<String, String>. Now map was not filled (there is a comment to remember to fix it).

Personally I do not like that exception design. Do you make in that manner becuase the any raised IOException is not catched in the buildstep? Note that workspace.act wraps the IOException into other IOException. Have a look to the hudson.tasks.ArtifactArchiver (to which this class I think has been inspired) on how to handles IOExceptions.

As I suggest to Jens this class should be refactored to be working correctly on slave node.

[nfalco79]

The other goal of refactor of the DroneRender is enable this class to be more testable.

[chrismathis]

@nfalco79 I did it that way because workspace.act wraps the IOException and adds it's own error message. That would hide the Uploader exception messages.

The ArtifactArchiver returns a Map<String,String> to feed it to ArtifactManager. Do you plan to return a Map<String,String> to feed it to something else that does the upload from the perform method? Will that be executed on the correct machine (master/slave)? Edit: found the usage of the map for run.addAction. Although I could not find where the information from the map adds something to Jenkins UI

If the IOExceptions are not caught at all, they fail the build. I think if failsAsUpload is false the build should not fail if there are IOExceptions with the packagedrone server.

[chrismathis]

We could add a UploadResult class to contain the Map<String,String> for the action and the Exception or problem flags (isEmptyUpload, uploadFailed)

[nfalco79]

@chrismathis you can unwrap the IOException until you get your UploaderException (extended by Empty....Exception and FailedUpload...Excption), otherwise other kind of issue was happens.

Anyway the UploadResult solution sound good

[chrismathis]

Added UploaderResult that reports back build status and artifact map back to master.

[nfalco79]

Please remember to use the correct code formatter (preferably only on the portion of the modified code)

[chrismathis]

Uh, sorry for that - changed (Also changed the setting for formatting changed lines)

[nfalco79]

good work if @ctron agree we can merge with a github squash commit.

[ctron]

Thanks for the great work everyone!

[ctron]

Should I make a new release?

[nfalco79]

Let consider the #19 that looks better instead of artifactId = filename and add coverage for V2/V3 implementations