Repository navigation
Conversation
Signed-off-by: prithvi <prithvisivasankar@gmail.com>
There was a problem hiding this comment.
Thanks @iprithv , the change overall looks good!
But the PR title should use a period instead of a colon( HDDS-16835. Remove unreachable endsWith branch in S3 ListObjects), otherwise the title check in CI will fail.
Also, CI has not run in your fork. I think you need to enable GitHub Actions in your fork and push again.
It would also be good to follow the PR template in the description
iprithv
changed the title
| */ | ||
| @Test | ||
| public void listExactPrefixWithTrailingDelimiter() throws OS3Exception, IOException { | ||
| setup("dir/", "dir/file1", "other"); |
There was a problem hiding this comment.
nit: I think this test does not really check the dir/ key.
dir/file1 alone already gives the common prefix dir/, so the
test passes even if dir/ is removed from setup.
Maybe use setup("dir/", "other") instead?
Signed-off-by: prithvi <prithvisivasankar@gmail.com>
|
thanks for the quick review! updatd the title, description and test, and enabled actions on the fork.. |
|
sure, done @Russole. thanks for checking this! |
|
|
||
| /** | ||
| * Key equal to prefix + delimiter (dir/ with prefix dir) is handled by the | ||
| * indexOf(delimiter) branch as a common prefix. An endsWith(delimiter) |
There was a problem hiding this comment.
one more small thing I just noticed
after this PR is merged, there is no endsWith in the code anymore, so the second sentence may confuse future readers.
F64116045
left a comment
F64116045
left a comment
There was a problem hiding this comment.
Thanks for the update. LGTM.
Left one minor comment, feel free to take it
Signed-off-by: prithvi <prithvisivasankar@gmail.com>
3 participants
else if (relativeKeyName.endsWith(delimiter))can never run. It is only reached whenrelativeKeyNamedoes not contain the delimiter, so it cannot end with it either.The case it was meant for (prefix
dir, keydir/) is already handled by theindexOf(delimiter)branch, which adds the common prefixdir/.removed the unreachable branch and added a regression test for that case.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16835
patch tested:
mvn -pl :ozone-s3gateway -am test -Dtest=TestBucketList -DskipShade -DskipRecon -DskipDocs
Result: Tests run: 33, Failures: 0, Errors: 0, Skipped: 0
Fork CI (
build-branchonHDDS-16835, commit77837c13c2): successhttps://github.com/iprithv/ozone/actions/runs/38065079180