You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/08/02 10:59:52 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #4628: Use Config System for Object Store Integration Tests
tustvold opened a new pull request, #4628:
URL: https://github.com/apache/arrow-rs/pull/4628
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Whilst working on https://github.com/apache/arrow-rs/pull/4239 I wanted a way to hook in additional config options for integration testing. We now have a first party config system, we should just use this for the tests instead of rolling a custom setup for tests. This is not only simpler, but is more flexible.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4628: Use Config System for Object Store Integration Tests
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4628:
URL: https://github.com/apache/arrow-rs/pull/4628#discussion_r1281748833
##########
object_store/src/aws/mod.rs:
##########
@@ -1067,153 +1067,9 @@ mod tests {
};
use bytes::Bytes;
use std::collections::HashMap;
- use std::env;
const NON_EXISTENT_NAME: &str = "nonexistentname";
- // Helper macro to skip tests if TEST_INTEGRATION and the AWS
- // environment variables are not set. Returns a configured
- // AmazonS3Builder
- macro_rules! maybe_skip_integration {
- () => {{
- dotenv::dotenv().ok();
-
- let required_vars = [
- "OBJECT_STORE_AWS_DEFAULT_REGION",
- "OBJECT_STORE_BUCKET",
- "OBJECT_STORE_AWS_ACCESS_KEY_ID",
- "OBJECT_STORE_AWS_SECRET_ACCESS_KEY",
- ];
- let unset_vars: Vec<_> = required_vars
- .iter()
- .filter_map(|&name| match env::var(name) {
- Ok(_) => None,
- Err(_) => Some(name),
- })
- .collect();
- let unset_var_names = unset_vars.join(", ");
-
- let force = env::var("TEST_INTEGRATION");
-
- if force.is_ok() && !unset_var_names.is_empty() {
- panic!(
- "TEST_INTEGRATION is set, \
- but variable(s) {} need to be set",
- unset_var_names
- );
- } else if force.is_err() {
- eprintln!(
- "skipping AWS integration test - set {}TEST_INTEGRATION to run",
- if unset_var_names.is_empty() {
- String::new()
- } else {
- format!("{} and ", unset_var_names)
- }
- );
- return;
- } else {
- let config = AmazonS3Builder::new()
- .with_access_key_id(
- env::var("OBJECT_STORE_AWS_ACCESS_KEY_ID")
- .expect("already checked OBJECT_STORE_AWS_ACCESS_KEY_ID"),
- )
- .with_secret_access_key(
- env::var("OBJECT_STORE_AWS_SECRET_ACCESS_KEY")
- .expect("already checked OBJECT_STORE_AWS_SECRET_ACCESS_KEY"),
- )
- .with_region(
- env::var("OBJECT_STORE_AWS_DEFAULT_REGION")
- .expect("already checked OBJECT_STORE_AWS_DEFAULT_REGION"),
- )
- .with_bucket_name(
- env::var("OBJECT_STORE_BUCKET")
- .expect("already checked OBJECT_STORE_BUCKET"),
- )
- .with_allow_http(true);
-
- let config = if let Ok(endpoint) = env::var("OBJECT_STORE_AWS_ENDPOINT") {
- config.with_endpoint(endpoint)
- } else {
- config
- };
-
- let config = if let Ok(token) = env::var("OBJECT_STORE_AWS_SESSION_TOKEN")
- {
- config.with_token(token)
- } else {
- config
- };
-
- let config = if let Ok(virtual_hosted_style_request) =
- env::var("OBJECT_STORE_VIRTUAL_HOSTED_STYLE_REQUEST")
- {
- config.with_virtual_hosted_style_request(
- virtual_hosted_style_request.trim().parse().unwrap(),
- )
- } else {
- config
- };
-
- config
- }
- }};
- }
-
- #[test]
- fn s3_test_config_from_env() {
Review Comment:
This test is no longer necessary as we test that from_env is hooked up as part of all tests, and s3_test_config_from_map below tests the various config keys
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4628: Use Config System for Object Store Integration Tests
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4628:
URL: https://github.com/apache/arrow-rs/pull/4628#discussion_r1281751380
##########
object_store/Cargo.toml:
##########
@@ -71,7 +71,6 @@ aws = ["cloud"]
http = ["cloud"]
[dev-dependencies] # In alphabetical order
-dotenv = "0.15.0"
Review Comment:
I couldn't see a compelling reason to keep this, I don't use it and I reckon few users are even aware of its existence, lets scream test removing it and see if anyone complains. It only impacts testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4628: Use Config System for Object Store Integration Tests
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4628:
URL: https://github.com/apache/arrow-rs/pull/4628#discussion_r1281749441
##########
.github/workflows/object_store.yml:
##########
@@ -82,12 +82,18 @@ jobs:
# Run integration tests
TEST_INTEGRATION: 1
EC2_METADATA_ENDPOINT: http://localhost:1338
+ AZURE_BUCKET: test-bucket
Review Comment:
As an added bonus this makes integration testing simpler, as there is no longer a requirement for the same bucket across all backends under test
##########
.github/workflows/object_store.yml:
##########
@@ -82,12 +82,18 @@ jobs:
# Run integration tests
TEST_INTEGRATION: 1
EC2_METADATA_ENDPOINT: http://localhost:1338
+ AZURE_BUCKET: test-bucket
Review Comment:
As an added bonus this makes integration testing simpler, as there is no longer a requirement for the same bucket name across all backends under test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org