Skip to content

Refactor deletefolder API#2017

Merged
Tulsishah merged 34 commits into
masterfrom
execute_e2e_tests_on_hns_implicit_dir
Jun 27, 2024
Merged

Refactor deletefolder API#2017
Tulsishah merged 34 commits into
masterfrom
execute_e2e_tests_on_hns_implicit_dir

Conversation

@Tulsishah

@Tulsishah Tulsishah commented Jun 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

  • Added the includeFolderAsPrefix parameter with a value of true, as required for HNS scenarios. We internally pass includeFolderAsPrefix true in ListObject method.
  • Refactored deleteFolder API. As previous logic was not logging DeleteObject request in case on FLAT bucket properly.
  • Added unit test for prefix_bucket_test
  • Also, included the enable-hns flag in the implicit-dir package to enable end-to-end testing for the delete folder functionality.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - Automated

@Tulsishah Tulsishah added the execute-integration-tests Run only integration tests label Jun 14, 2024
@codecov

codecov Bot commented Jun 14, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.15%. Comparing base (3f8314a) to head (0297242).
Report is 3 commits behind head on master.

Files Patch % Lines
internal/fs/inode/dir.go 58.33% 4 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2017      +/-   ##
==========================================
- Coverage   71.99%   71.15%   -0.85%     
==========================================
  Files          99       99              
  Lines       10870    11035     +165     
==========================================
+ Hits         7826     7852      +26     
- Misses       2712     2854     +142     
+ Partials      332      329       -3
Flag Coverage Δ
unittests 71.15% <64.28%> (-0.85%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tulsishah Tulsishah force-pushed the execute_e2e_tests_on_hns_implicit_dir branch from 108554d to cafdb95 Compare June 18, 2024 08:58
@Tulsishah Tulsishah changed the title Adding new flag set in implicit dir package for HNS Passing includeFolderAsPrefix true in case of HNS and enable e2e tests for implicit dir Jun 18, 2024
@Tulsishah Tulsishah force-pushed the execute_e2e_tests_on_hns_implicit_dir branch from 5cc2522 to e9a5a6e Compare June 18, 2024 11:01
@Tulsishah Tulsishah marked this pull request as ready for review June 18, 2024 11:21
@Tulsishah Tulsishah requested a review from a team as a code owner June 18, 2024 11:21
ankitaluthra1
ankitaluthra1 suggested changes Jun 20, 2024
View reviewed changes
Comment thread internal/storage/bucket_handle.go Outdated
Comment thread tools/integration_tests/util/setup/setup.go Outdated
@Tulsishah Tulsishah force-pushed the execute_e2e_tests_on_hns_implicit_dir branch from 7aa2d59 to 6936cde Compare June 20, 2024 10:02
@Tulsishah Tulsishah requested a review from ankitaluthra1 June 20, 2024 11:13
@Tulsishah Tulsishah changed the title Passing includeFolderAsPrefix true in case of HNS and enable e2e tests for implicit dir Refactor deletefolder API Jun 21, 2024
@Tulsishah Tulsishah force-pushed the execute_e2e_tests_on_hns_implicit_dir branch from d36a935 to b17d821 Compare June 21, 2024 17:16
ankitaluthra1
ankitaluthra1 suggested changes Jun 25, 2024
View reviewed changes
Comment thread internal/fs/inode/dir.go
Comment thread internal/fs/inode/dir.go Outdated
@Tulsishah Tulsishah force-pushed the execute_e2e_tests_on_hns_implicit_dir branch from abf0c6e to d657176 Compare June 26, 2024 04:48
@Tulsishah Tulsishah requested a review from kislaykishore as a code owner June 26, 2024 04:48
@Tulsishah Tulsishah requested a review from ankitaluthra1 June 26, 2024 05:42
kislaykishore
kislaykishore requested changes Jun 26, 2024
View reviewed changes

@kislaykishore kislaykishore left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • Please explain the rationale behind the refactoring.
  • The PR description talks about setting includeFolderAsPrefix as true. I'm not sure which method call accepts this value and is set to true. Could you explain that.

Comment thread internal/config/yaml_parser.go Outdated
Comment thread internal/fs/inode/dir.go Outdated
Comment thread internal/fs/inode/dir.go Outdated
Comment thread internal/fs/inode/dir.go
Comment thread internal/fs/inode/dir.go Outdated
Comment thread internal/gcsx/prefix_bucket_test.go Outdated
Comment thread ...egration_tests/util/setup/implicit_and_explicit_dir_setup/implicit_and_explicit_dir_setup.go Outdated
Comment thread internal/config/yaml_parser.go
Comment thread internal/gcsx/prefix_bucket_test.go Outdated
Comment thread internal/gcsx/prefix_bucket_test.go Outdated
@Tulsishah

Tulsishah commented Jun 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author
  • Please explain the rationale behind the refactoring.
  • The PR description talks about setting includeFolderAsPrefix as true. I'm not sure which method call accepts this value and is set to true. Could you explain that.

Added more info

@Tulsishah Tulsishah requested a review from kislaykishore June 26, 2024 09:39
kislaykishore
kislaykishore requested changes Jun 27, 2024
View reviewed changes
Comment thread internal/storage/bucket_handle_test.go Outdated
Comment thread ...egration_tests/util/setup/implicit_and_explicit_dir_setup/implicit_and_explicit_dir_setup.go Outdated
Comment thread tools/integration_tests/util/setup/setup.go Outdated
Comment thread tools/integration_tests/operations/operations_test.go Outdated
@Tulsishah Tulsishah requested a review from kislaykishore June 27, 2024 08:06
kislaykishore
kislaykishore approved these changes Jun 27, 2024
View reviewed changes

@kislaykishore kislaykishore left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please get it reviewed by someone else on the team as well.

charith87
charith87 reviewed Jun 27, 2024
View reviewed changes
Comment thread tools/integration_tests/util/setup/setup.go
@Tulsishah Tulsishah merged commit 3995bfb into master Jun 27, 2024
@Tulsishah Tulsishah deleted the execute_e2e_tests_on_hns_implicit_dir branch July 19, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kislaykishore kislaykishore kislaykishore approved these changes

@charith87 charith87 charith87 approved these changes

+1 more reviewer

@ankitaluthra1 ankitaluthra1 ankitaluthra1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

execute-integration-tests Run only integration tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Tulsishah @kislaykishore @charith87 @ankitaluthra1