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

Add ZstdCompressor to S3 Plugin and Fix Tests According to Maintainer's Feedback #440

Closed
wants to merge 7 commits into from

Conversation

ddukbg
Copy link
Contributor

@ddukbg ddukbg commented Oct 25, 2024

This Pull Request addresses the maintainer's feedback regarding the addition of the ZstdCompressor to the S3 output plugin.

Changes Made:

  1. Removed Duplicate ZstdCompressor Class:

    • Deleted lib/fluent/plugin/s3_compressor_zstd.rb as the ZstdCompressor class was already defined in out_s3.rb, eliminating duplication.
  2. Added Tests for ZstdCompressor in test_out_s3.rb:

    • Moved the tests related to ZstdCompressor from test_in_s3.rb to test_out_s3.rb since they pertain to the output plugin.
    • Added test cases test_configure_with_mime_type_zstd and test_write_with_zstd to verify the proper functioning of ZstdCompressor.
  3. Ensured ZstdCompressor is Properly Registered:

    • Verified that ZstdCompressor is correctly registered in the COMPRESSOR_REGISTRY within out_s3.rb.
  4. Fixed Test Failures:

    • Adjusted the s3path in test_write_with_zstd to match the expected file extension for zstd compression, resolving the test failure.
  5. Resolved Gem Build Error:

    • Removed s3_compressor_zstd.rb from the Git index to fix the gem build error caused by the file being listed but not present.

Notes:

  • All changes have been made according to the maintainer's comments.
  • Tests have been updated and all pass successfully.
  • Documentation and code comments have been updated accordingly.

Please review the changes, and let me know if any further adjustments are needed.

dependabot bot and others added 7 commits October 18, 2024 18:30
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: yongwoo.kim <[email protected]>
Before:
- name: Install dependencies
  run: gem install bundler rake

After:
- name: Install dependencies
  run: gem install rake

Signed-off-by: ddukbg <[email protected]>
…mments

Moved ZstdCompressor tests from test_in_s3.rb to test_out_s3.rb as they relate to the out_s3 plugin.

Signed-off-by: ddukbg <[email protected]>
…omments

Added tests for ZstdCompressor to test_out_s3.rb following the maintainer's suggestions.

Signed-off-by: ddukbg <[email protected]>
@ddukbg ddukbg closed this Oct 25, 2024
@ddukbg
Copy link
Contributor Author

ddukbg commented Oct 25, 2024

Oh, I mistakenly submitted the PR incorrectly.

I accidentally submitted the wrong PR. I'm working on PR#439.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant