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

[MM-57034]: Check for s3 object is File or Directory #26837

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

apshada
Copy link
Contributor

@apshada apshada commented Apr 22, 2024

Summary

Fixes: #26742

Ticket Link

JIRA :- MM-57034

Screenshots

Release Note

NONE

apshada and others added 5 commits January 25, 2024 17:22
[pull] master from mattermost:master
[pull] master from mattermost:master
[pull] master from mattermost:master
[pull] master from mattermost:master
@mm-cloud-bot mm-cloud-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 22, 2024
@mattermost-build
Copy link
Contributor

Hello @apshada,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@hanzei hanzei added the 2: Dev Review Requires review by a developer label Apr 23, 2024
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test. Look at s3store_test.go. Thanks!

@apshada
Copy link
Contributor Author

apshada commented Apr 24, 2024

Please add a test. Look at s3store_test.go. Thanks!

@agnivade Test Cases to be included in filtesstore_test.go here?

@agnivade
Copy link
Member

You can add an entirely new test. Thanks!

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@apshada apshada requested a review from agnivade May 5, 2024 17:37
count++
}
// Check if only one item was returned and it matches the path prefix
if count == 1 && len(paths) > 0 && strings.TrimRight(path, "/") == strings.TrimRight(paths[0], "/") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to do strings.TrimRight for paths[0] because it's already done inside the for loop.

_, err = fileBackend.WriteFile(bytes.NewReader(b), path1)
require.NoError(t, err, "Failed to write file1 to S3")

_, err = fileBackend.listDirectory("", false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the exported method ListDirectory.

require.NoError(t, err, "Failed to write file1 to S3")

_, err = fileBackend.listDirectory("", false)
require.Error(t, err, "readdir 19700101/: file does not exist []")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for the actual error type apart from just asserting for the error.

var pErr *fs.PathError
assert.True(t, errors.As(err, &pErr), "error is not of type fs.PathError")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@apshada apshada requested a review from agnivade May 6, 2024 16:03
Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@isacikgoz
Copy link
Member

/update-branch

@agnivade agnivade added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels May 27, 2024
@agnivade
Copy link
Member

/e2e-test

@unified-ci-app
Copy link
Contributor

E2E test triggered successfully for PR #26837. The corresponding commit's status check will be available shortly.

Copy link

E2E test run is starting for commit 81124171a238b359c52c50c5cf7f68c696c6d04c.
You can check its progress by either:

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Contributor Lifecycle/1:stale release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filestore: S3 implementation should return an error on ListDirectory method if the object is a file
6 participants