Support for pmtiles S3 urls #1477
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
This seems to be almost ready! The biggest challenge is ... sadly... how to test it? I wonder if we should set up some public s3 bucket with some small dummy pmtiles and use it in all the testing? @birkskyum @louwers any thoughts on where we can place it? P.S. please rebase |
|
https://pmtilestest.s3.eu-central-1.amazonaws.com/cb_2018_us_zcta510_500k.pmtiles This is in a MapLibre owned bucket. |
|
@louwers why is a public readonly s3 bucket with only a small downloadable is a bad idea? Seems like it would be no different than exposing that same bucket with the http interface, and it would let us test s3 API |
|
@nyurik I misunderstood and removed my earlier comment, I thought we need a S3 bucket for various kinds of test assets. Please use the link I provided! :) |
|
@louwers we do want to upload a bunch of test assets to an s3 bucket (by an admin), but that bucket would be used as a readonly source when running CI tests |
|
@louwers the link you posted above - do you have a full |
|
That would be |
alamminsalo
force-pushed
the
pmtiles_s3
branch
from
4b5196d to
5a18b12
Compare
|
I migrated to using pmtiles aws-s3-async feature, which uses aws-sdk-s3 and aws-config libraries. Edit: also added tests for the S3 backed based on your suggestions |
|
The testcase for
You likely need the same environment variables here: martin/.github/workflows/grcov.yml Line 62 in 321af56 |
|
Hi @CommanderStorm and @nyurik, all builds and tests have passed successfully on this run: https://github.com/Youssef-Harby/martin/actions/runs/15181986547 ... When you have a moment, could you please review and let me know the preferred next steps? I'm happy to open a new PR if needed, or feel free to take the necessary changes directly from my fork/branch .. whichever works best for you. Thanks! |
|
I noticed that Martin was having difficulty connecting to S3-compatible services like MinIO when using S3 URLs. The issue was that S3-compatible services often require path-style addressing rather than virtual host addressing. I have added some modifications to check for The implementation reads the environment variable once during client initialization and applies the setting only when needed. I don't have a public s3 pmtiles with custom endpoint url to develop a test case .. so will need your feedback on this.. |
There was a problem hiding this comment.
Thanks for working on this! Overall seems reasonable. One concern I have is that all of a sudden we rely on the environment variables for pmtiles, while PG sources have a corresponding config value. Does S3 allow query-style params? We could remove them too of course:
s3://bucket/path/to/tiles.pmtiles?AWS_NO_CREDENTIALS=1
Co-authored-by: Yuri Astrakhan <[email protected]>
Co-authored-by: Yuri Astrakhan <[email protected]>
Co-authored-by: Yuri Astrakhan <[email protected]>
| #[cfg(any(feature = "pmtiles", feature = "mbtiles", feature = "cog"))] | ||
| fn is_url(s: &str, extension: &[&str]) -> bool { | ||
| if s.starts_with("http") { | ||
| if s.starts_with("http") || s.starts_with("s3") { |
There was a problem hiding this comment.
i keep wondering if we even need this optimization - if this function is only used for parsing, we might as well just parse the URL and check the schema. Also, why do we parse the url here, and also have parse_url? I probably did it a while back, but not sure if this duplicity is needed
Not to my knowlege. I can add a config option for both if you'd like. |
|
|
||
| #[cfg(feature = "pmtiles")] | ||
| #[error(r"PMTiles error {0} processing {1}")] | ||
| #[error(r"PMTiles error {0:?} processing {1}")] |
There was a problem hiding this comment.
this is a bit of a surprising change, but not a biggie
…e}` (#1848) this PR is the followup talked about in #1477 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Yuri Astrakhan <[email protected]> Co-authored-by: Copilot <[email protected]>
Added support for s3-urls to config.
This is a bit of a rough sketch currently.
The aws config struct does not seem to support sso-login credentials out of the box, I needed to add the credentials to environment variables along with AWS_REGION to get it working.
Resolves #1171