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

Support for pmtiles S3 urls #1477

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

Conversation

alamminsalo
Copy link

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.

CommanderStorm

This comment was marked as resolved.

@nyurik
Copy link
Member

nyurik commented Oct 22, 2024

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

@louwers
Copy link
Contributor

louwers commented Oct 22, 2024

@nyurik
Copy link
Member

nyurik commented Oct 22, 2024

@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

@louwers
Copy link
Contributor

louwers commented Oct 22, 2024

@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! :)

@nyurik
Copy link
Member

nyurik commented Oct 22, 2024

@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

@nyurik
Copy link
Member

nyurik commented Oct 22, 2024

@louwers the link you posted above - do you have a full s3:// path to it?

@louwers
Copy link
Contributor

louwers commented Oct 22, 2024

That would be s3://pmtilestest/cb_2018_us_zcta510_500k.pmtiles

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

few nits about formatting in the docs

docs/src/sources-files.md Outdated Show resolved Hide resolved
docs/src/sources-files.md Outdated Show resolved Hide resolved
docs/src/sources-files.md Outdated Show resolved Hide resolved
@alamminsalo
Copy link
Author

alamminsalo commented Nov 13, 2024

I migrated to using pmtiles aws-s3-async feature, which uses aws-sdk-s3 and aws-config libraries.
They have better credentials configuration flexibility (eg. can use SSO credentials) and does not need the AWS_REGION environment variable to be set explicitly (can be also defined in profiles).
The support for them seems to have landed in pmtiles 0.11.

Edit: also added tests for the S3 backed based on your suggestions

@CommanderStorm
Copy link
Collaborator

The testcase for grcov is failing..

image

You likely need the same environment variables here:

CARGO_INCREMENTAL: '0'

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Nov 16, 2024

@nyurik not sure what our policy on this is:
Which testcases should be added to

>&2 echo "***** Test server response for PMTiles source *****"

Is this one too specific or still fine to add?

@CommanderStorm
Copy link
Collaborator

image

Seems that even after adressing 94f8d43, there is one other issue that prevents this being merged.

I or @alamminsalo will need to investigate where this dying is coming from.

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.

4 participants