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

feat: add support for S3 as external data source #981

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

margau
Copy link
Contributor

@margau margau commented Nov 28, 2023

This PR adds support for S3:

  • the orchestrator is a central S3 proxy
  • currently, loading custom dictionaries from a S3 bucket is supported
  • this can be enhanced to e.g. ipinfo parts

With S3 as alternative to local storage, Kubernetes-Like deployment (see e.g. #891) are made easier, as the components itself don't need persistent storage.

Still open:

  • Documentation
  • possibly more tests

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (9c4ce55) 84.84% compared to head (b218267) 84.68%.
Report is 1 commits behind head on main.

Files Patch % Lines
common/s3/root.go 64.44% 16 Missing ⚠️
orchestrator/clickhouse/http.go 85.18% 4 Missing ⚠️
common/schema/config.go 72.72% 3 Missing ⚠️
cmd/orchestrator.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
- Coverage   84.84%   84.68%   -0.16%     
==========================================
  Files         186      189       +3     
  Lines        7238     7322      +84     
  Branches       38       38              
==========================================
+ Hits         6141     6201      +60     
- Misses       1095     1119      +24     
  Partials        2        2              
Flag Coverage Δ
unittests 84.68% <73.62%> (-0.16%) ⬇️

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.

@margau margau force-pushed the feat/s3-custom-dict branch 5 times, most recently from ddef829 to 9f1820f Compare December 14, 2023 11:30
@vincentbernat
Copy link
Member

Note I didn't have a look as I understand this is still work in progress. Tell me when you need some comments.

@margau
Copy link
Contributor Author

margau commented Dec 15, 2023

Hi,
feature-side, I'd consider this ready and would like to get a review.
The only thing missing is better test coverage, and the documentation of the final config structure.

@vincentbernat
Copy link
Member

vincentbernat commented Dec 24, 2023

Sorry for taking some time. I am fine with the feature, however I have a few remarks on the implementation.

  1. At first, I thought the way the configuration is split in two parts was strange. But I understand you want to reuse configuration for several dicts (or later more stuff), so ack for this.
  2. There is a mechanism to handle polymorphism, take a look at use of helpers.ParametrizedConfiguration* invocations. You can then have file, http, s3, and s3mock backends. The later will use a mock version of the S3 component to avoid the Mock attribute in it. This is pretty clear in my head, if it is not for you, feel free to ignore, I'll add a commit to handle that.

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.

None yet

2 participants