Support AWS_ENDPOINT_URL #84
Conversation
Closes #83.
aykut-bozkurt
force-pushed
the
aykut/aws-url-endpoint
branch
from
b0dc4e0 to
fcd0d85
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 92.15% 92.18% +0.02%
==========================================
Files 71 71
Lines 9040 9087 +47
==========================================
+ Hits 8331 8377 +46
- Misses 709 710 +1 ☔ View full report in Codecov by Sentry. |
src/arrow_parquet/uri_utils.rs
Outdated
| // AWS_ALLOW_HTTP | ||
| if let Ok(aws_allow_http) = std::env::var("AWS_ALLOW_HTTP") { | ||
| aws_s3_builder = aws_s3_builder | ||
| .with_allow_http(aws_allow_http.parse().unwrap_or_else(|e| panic!("{}", e))); |
There was a problem hiding this comment.
can the unwrap_or_else be expect!?
There was a problem hiding this comment.
expect(format!()) gives lint errors but we might use unwrap or give additional context with unwrap_or_else
src/arrow_parquet/uri_utils.rs
Outdated
| aws_s3_builder = aws_s3_builder | ||
| .with_allow_http(aws_allow_http.parse().unwrap_or_else(|e| panic!("{}", e))); | ||
| }; | ||
|
|
There was a problem hiding this comment.
probably good to add some context in a comment on why we have this additional logic
at this point, I don't have a very good mental model of what object_store does and does not do for us.
There was a problem hiding this comment.
Added comment to the function.
Allowed passing the environment variables to object_store as it is. I was not passing them since we do not read all corresponding configs from the config file (at fallback) but now I see no downside passing all the env vars (except some of the configs, that can be read from environment, cannot be read from the config file. Supported configs by both environment and config files are documented at readme). (context: object_store can not read from config files)
| if let Some(aws_region) = sdk_config.region() { | ||
| aws_s3_builder = aws_s3_builder.with_region(aws_region.as_ref()); | ||
| } | ||
|
|
There was a problem hiding this comment.
does this code also cover other paths like using the EC2 metadata server to obtain credentials?
There was a problem hiding this comment.
When all required environment variables are configured to auth via EC2 server, object_store supports it. But we do not read some of the environment variables from the config.
| run: | | ||
| cargo install --locked [email protected] | ||
| cargo pgrx init --pg${{ env.PG_MAJOR }} $(which pg_config) | ||
| cargo pgrx init --pg${{ env.PG_MAJOR }} /usr/lib/postgresql/${{ env.PG_MAJOR }}/bin/pg_config |
There was a problem hiding this comment.
Latest ubuntu CI runner is probably changed and default postgres version is different. To be stable on that, lets specify pg_config absolute path
marcoslot
left a comment
marcoslot
left a comment
There was a problem hiding this comment.
Thanks, I find the new flow easier to follow.
AWS_ENDPOINT_URL, e.g. you can setAWS_ENDPOINT_URL=http://localhost:9000andAWS_ALLOW_HTTP=truefor local MinIO Server.AWS_SESSION_TOKENwithout documentation and tests. Added them.As a side note, not automatically
AssumeRoleto fetch the token, but when passed from environment or config, authentication succeeds with temporary token.Closes #83.