DEVPROD-13056: fix s3 put skip_existing failures #8510
Conversation
drichmdb
force-pushed
the
DEVPROD-13056
branch
3 times, most recently
from
441e21a to
40ed804
Compare
agent/command/s3_put.go
Outdated
| @@ -422,6 +424,12 @@ retryLoop: | |||
| return errors.Wrapf(err, "checking if file '%s' exists", remoteName) | |||
| } | |||
| if exists { | |||
| if s3pc.preservePath { | |||
There was a problem hiding this comment.
nit -- since we aren't using this value anyway, can we just keep a skippedFilesCount instead? Then we don't need an if/else here. (Seems like we might be able to do that real quick for uploadedFiles too if you want to make them consistent, but I think it's fine either way)
There was a problem hiding this comment.
We can't make that change for uploadedFiles because they are used by the attachFiles function, but I could for skippedFiles quickly!
| remoteFile := fmt.Sprintf("tests/%s/%s", t.Name(), id) | ||
|
|
||
| cmd := s3PutFactory() | ||
| params := map[string]any{ |
| content, err := io.ReadAll(got) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, payload, content) |
There was a problem hiding this comment.
Can you add a little comment here, like "verify that file content wasn't overwritten by the second file"
drichmdb
force-pushed
the
DEVPROD-13056
branch
2 times, most recently
from
df22233 to
ff5cff3
Compare
| aws_key: $AWS_ACCESS_KEY_ID | ||
| aws_secret: $AWS_SECRET_ACCESS_KEY |
There was a problem hiding this comment.
Are these meant to be different than the ones used on line 54 and 55? I don't have time for a full review so I may be missing something.
There was a problem hiding this comment.
I'm not sure where those are coming from honestly -- I'm using the credentials we get from assume role at the beginning of the task!
|
|
||
| if processedCount != len(filesList) && !s3pc.skipMissing { | ||
| logger.Task().Infof("Attempted to upload %d files, %d successfully uploaded.", len(filesList), processedCount) | ||
| return errors.Errorf("uploaded %d files of %d requested", processedCount, len(filesList)) |
There was a problem hiding this comment.
I think this edit is wrong -- it should still be using length of uploaded files
This commit fixes s3.put's skip_existing parameter to not always fail when a remote file already exists. At the end of s3.put, we check that the length of uploadedFiles matches the length of fileList. If the lengths aren't equal, the command should fail because there was at least one upload error. skip_existing did not append to this list, but these skipped files were expected and should not be errors. To fix this, I added a new slice that tracks files we've skipped separately. The end check now considers both uploaded and skipped files to determine if there was an error. I've also included a new integration test that checks s3.put with skip_existing set to true.
drichmdb
force-pushed
the
DEVPROD-13056
branch
from
ff5cff3 to
bc1cafc
Compare
DEVPROD-13056
Description
This commit fixes s3.put's skip_existing parameter to not always fail when a remote file already exists. At the end of s3.put, we check that the length of uploadedFiles matches the length of fileList. If the lengths aren't equal, the command should fail because there was at least one upload error. skip_existing did not append to this list, but these skipped files were expected and should not be errors.
To fix this, I added a new slice that tracks files we've skipped separately. The end check now considers both uploaded and skipped files to determine if there was an error.
I've also included a new integration test that checks s3.put with skip_existing set to true.