fix: read error from upload artifacts execution.
What does this MR do?
Fixes a bug when an error occurs during the "artifacts upload" stage.
Why was this MR needed?
During a failure in the upload artifacts step of a build jobs were not failing. This is a problem.
I have tested my patch with a local build and it makes the job fail if an error occurs during artifact uploading execution:
- Example before patch https://gitlab.com/paulrbr/paulrbr.gitlab.io/-/jobs/28482190 (forcing to always return an error on upload artifact)
FATAL: invalid argument
Job succeeded
- Example after patch https://gitlab.com/paulrbr/paulrbr.gitlab.io/-/jobs/28483043 (forcing to always return an error on upload artifact)
FATAL: invalid argument
ERROR: Job failed: exit code 1
Are there points in the code the reviewer needs to double check?
Help would be appreciated to create a test case for failed uploads
builds.
Does this MR meet the acceptance criteria?
-
Documentation created/updated - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Branch has no merge conflicts with master
(if you do - rebase it please)
What are the relevant issue numbers?
Fixes #2584 (closed)
Merge request reports
Activity
mentioned in issue #2584 (closed)
@nick.thomas Could you please review?
added ~147498 typebug labels
Thanks for this MR @paulrbr ! This is a very annoying bug :/
- Resolved by Paul 🐻
added 3 commits
-
04a1054e...9a62dbfc - 2 commits from branch
gitlab-org:master
- 0fa12402 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
-
04a1054e...9a62dbfc - 2 commits from branch
added 1 commit
- 170f7bdc - fix: read error from upload artifacts execution. Fixes #2584 (closed).
added 2 commits
- c4e967d5 - test: Add a failing test on artifact upload error
- ee717ba5 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
added 2 commits
- 28fe7093 - test: Add a failing test on artifact upload error
- 61ecf148 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
Thanks @paulrbr, looks good to me. The test is especially appreciated!
The method still feels too complicated - perhaps the
when
logic could be extracted into a method oftype Artifact
- but that can be deferred until a later MR.@tmaczukin can you review as maintainer?
Edited by Nick Thomasassigned to @tmaczukin
I agree with @nick.thomas - we should simplify the
when
part and let's do this now.I propose to add this in
common/network.go
:func (aw ArtifactWhen) OnSuccess() bool { return aw == "" || aw == ArtifactWhenOnSuccess || aw == ArtifactWhenAlways } func (aw ArtifactWhen) OnFailure() bool { return aw == ArtifactWhenOnFailure || aw == ArtifactWhenAlways } func (a Artifact) ShouldUpload(state error) bool { return (state == nil && a.When.OnSuccess()) || (state != nil && a.When.OnFailure()) }
Then we can change the
executeUploadArtifacts()
function to:func (b *Build) executeUploadArtifacts(ctx context.Context, state error, executor Executor) (uploadError error) { for _, artifact := range b.Artifacts { if artifact.ShouldUpload(state) { uploadError = b.executeStage(ctx, BuildStageUploadArtifacts, executor) } } return }
Notice that here we're also declaring
uploadError
in function header and skipping theif uploadError != nil { err = uploadError }
part at the end of the function.Rest looks good. After adding changes described above we can merge it :)
changed milestone to %9.5
assigned to @paulrbr
@tmaczukin thanks for the review.
Agreed with simplifying the
when
part.However for the uploadError variable I feel we'd make the method difficult to read. Of course I mentioned there's always only a size
1
slice, for now so it doesn't matter, but looking at the code I'm afraid someone will still think as @nick.thomas thought:there's another problem - say artifact 1 fails to upload, and we then upload artifact 2 successfully. In this case, err is overwritten with nil in the second iteration, and the job is marked as a success.
If you prefer to keep the variable as in header + remove condition at the end, just say the word :).
Edited by Paul 🐻added 5 commits
-
61ecf148...2b9142c0 - 3 commits from branch
gitlab-org:master
- 50b3cdfc - test: Add a failing test on artifact upload error
- 90624f89 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
-
61ecf148...2b9142c0 - 3 commits from branch
added 1 commit
- 90364371 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
@tmaczukin changes updated with your comment and rebased on
master
.- Resolved by Tomasz Maczukin
added 6 commits
-
90364371...f6178fdd - 4 commits from branch
gitlab-org:master
- ea8ff80b - test: Add a failing test on artifact upload error
- dd1ded65 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
-
90364371...f6178fdd - 4 commits from branch
Hi @nick.thomas and @tmaczukin do you think we will be able yo integrate this fix into 9.5 still? I think we've gone through all discussions now. Thanks!
assigned to @tmaczukin
@paulrbr One more thing. Let's add a test in
common/network_test.go
to check ifShouldUpload
works properly:func doTestArtifactShouldUpload(t *testing.T, when ArtifactWhen, stateOK, expected bool) { artifact := Artifact{When: when} var state error if !stateOK { state = errors.New("Test error") } result := artifact.ShouldUpload(state) if expected { assert.True(t, result, "ShouldUpload should return true for when=%v and state=%v", when, state) } else { assert.False(t, result, "ShouldUpload should return false for when=%v and state=%v", when, state) } } func TestArtifact_ShouldUpload(t *testing.T) { examples := []struct { when ArtifactWhen stateOK bool expected bool }{ {when: "", stateOK: true, expected: true}, {when: ArtifactWhenOnSuccess, stateOK: true, expected: true}, {when: ArtifactWhenOnFailure, stateOK: true, expected: false}, {when: ArtifactWhenAlways, stateOK: true, expected: true}, {when: "", stateOK: false, expected: false}, {when: ArtifactWhenOnSuccess, stateOK: false, expected: false}, {when: ArtifactWhenOnFailure, stateOK: false, expected: true}, {when: ArtifactWhenAlways, stateOK: false, expected: true}, } for _, example := range examples { doTestArtifactShouldUpload(t, example.when, example.stateOK, example.expected) } }
If you will not be able to add this until tomorrow, I'll do it myself and merge this into 9.5 :)
added 1 commit
- 0447319d - test: add a test for new ShouldUpload function
added 5 commits
-
0447319d...0a0ae247 - 2 commits from branch
gitlab-org:master
- 7514fd2e - test: Add a failing test on artifact upload error
- 9eec92c8 - fix: read error from upload artifacts execution. Fixes #2584 (closed).
- fccb4a64 - test: add a test for new ShouldUpload function
Toggle commit list-
0447319d...0a0ae247 - 2 commits from branch
added 1 commit
- 031f7982 - test: add a test for new ShouldUpload function
added 1 commit
- 97c1bef9 - test: add a test for new ShouldUpload function
- Resolved by Paul 🐻
- Resolved by Paul 🐻
added 1 commit
- d318e691 - test: add a test for new ShouldUpload function
added 1 commit
- d318e691 - test: add a test for new ShouldUpload function
Thanks for the extra test case @tmaczukin I've added a commit with it.
added 1 commit
- d318e691 - test: add a test for new ShouldUpload function
added 1 commit
- d318e691 - test: add a test for new ShouldUpload function
added 1 commit
- fc554083 - test: add a test for new ShouldUpload function
added 1 commit
- fc554083 - test: add a test for new ShouldUpload function
Hi @tmaczukin, MR is all set now.
mentioned in commit 19ab26c3
mentioned in issue gitlab#22711