Make rustls default across all packages #1097

wjones127 · 2023-01-25T03:41:00Z

Description

Clean up our tls options so that we can run cargo test at the top level again.

Also fixed clippy warnings that had accumulated.

Related Issue(s)

closes cargo test does not run due to tls conflict #985

Documentation

aws/delta-checkpoint/Cargo.toml

delta-inspect/Cargo.toml

houqp · 2023-01-25T04:05:58Z

Thank you @wjones127 for taking a stab at this!

wjones127 · 2023-01-26T03:38:51Z

rust/src/operations/create.rs

@@ -333,8 +333,12 @@ mod tests {
    #[tokio::test]
    async fn test_create_local_relative_path() {
        let table_schema = get_delta_schema();
-        let name = Alphanumeric.sample_string(&mut rand::thread_rng(), 16);
-        let table = DeltaOps::try_from_uri(format!("./{}", name))
+        let tmp_dir = TempDir::new_in(".", "tmp_").unwrap();


These tests were creating directories without cleaning them up.

Thanks for cleaning that up!

wjones127 · 2023-01-26T03:39:17Z

glibc_version/src/lib.rs

@@ -4,7 +4,7 @@
 //!
 //! # Example
 //!
-//! ```
+//! ```no_run


Can't run this on MacOS, and wasn't sure how to opt out of doc test based on platform.

wjones127 · 2023-02-07T04:25:56Z

Well I've figured out most of the issues, but the Windows build I haven't figured out 😢

houqp · 2023-02-07T05:43:38Z

@wjones127 have you tried kicking off the windows build without loading the previous build cache?

delta-inspect/Cargo.toml

python/Cargo.toml

wjones127 · 2023-02-11T03:02:33Z

@houqp Thanks, clearing the cache worked!

wjones127 · 2023-02-11T18:28:21Z

rust/src/storage/config.rs

@@ -230,16 +216,10 @@ impl ObjectStoreKind {
            }),
            #[cfg(feature = "gcs")]
            ObjectStoreKind::Google => {
-                let store = GoogleCloudStorageBuilder::new()
+                let store = GoogleCloudStorageBuilder::from_env()


The integration tests were failing for GCP without this change. In the tests, we configure GCP through the GOOGLE_SERVICE_ACCOUNT environment variable, but instead this code was creating a GCS storage with default configuration.

Is there a reason we shouldn't be picking up the environment variables first? I assume that the storage options override them. @roeap

That must have been a bug, or at least an inconsistency. I think for S3 and Azure we use from_env as well. That said., this is actually a point I struggled with a bit when writing this. Mainly for S3 and Azure where we have many different ways of authorising. One scenario in particular was troubling me. In case the environment contains enough information for the store to find a credential, and the storage options contain another complete set of config for a credential. In this case object_store will pick the credential based on the order they are checked in the builder, which does not necessarily give precedence to the one in the options, which I assume is what the user would want.

The initial approach was to try and create one only from the storage_options, and if we fail, load from environment and from options. TO be honest, I do not remember what the specific issue was why I abandoned this approach, but I do remember, that the special handling of allow_http is definitely related. In the end I guess, I though in scenarios where tables in multiple locations are managed people should be careful what to put in the env...

That said, anything in storage options will overwrite any info parsed from the environment.

roeap

Great work - will be nice running cargo from the root again 😆 .

roeap · 2023-02-11T21:02:17Z

rust/src/operations/create.rs

@@ -333,8 +333,12 @@ mod tests {
    #[tokio::test]
    async fn test_create_local_relative_path() {
        let table_schema = get_delta_schema();
-        let name = Alphanumeric.sample_string(&mut rand::thread_rng(), 16);
-        let table = DeltaOps::try_from_uri(format!("./{}", name))
+        let tmp_dir = TempDir::new_in(".", "tmp_").unwrap();


Thanks for cleaning that up!

# Description Clean up our tls options so that we can run `cargo test` at the top level again. Also fixed clippy warnings that had accumulated. # Related Issue(s) - closes delta-io#985 # Documentation

github-actions bot added binding/python Issues for the Python package binding/ruby delta-checkpoint delta-inspect rust storage/aws AWS S3 storage related labels Jan 25, 2023

houqp reviewed Jan 25, 2023

View reviewed changes

aws/delta-checkpoint/Cargo.toml Outdated Show resolved Hide resolved

houqp reviewed Jan 25, 2023

View reviewed changes

delta-inspect/Cargo.toml Outdated Show resolved Hide resolved

github-actions bot added the binding/rust Issues for the Rust crate label Jan 25, 2023

wjones127 changed the title ~~Refactor rustls features so workspace can be checked.~~ Make rustls default across all packages Jan 25, 2023

github-actions bot added the glibc_version label Jan 26, 2023

wjones127 commented Jan 26, 2023

View reviewed changes

wjones127 mentioned this pull request Feb 6, 2023

Compiler failure on clean build of rust code #1132

Closed

wjones127 force-pushed the fix/cargo-check-working branch from 46178df to 693361a Compare February 7, 2023 02:01

wjones127 marked this pull request as ready for review February 7, 2023 04:25

wjones127 requested review from xianwill, rtyler, mosyp, fvaleye and roeap as code owners February 7, 2023 04:25

wjones127 requested a review from houqp February 7, 2023 04:25

houqp reviewed Feb 7, 2023

View reviewed changes

delta-inspect/Cargo.toml Show resolved Hide resolved

houqp reviewed Feb 7, 2023

View reviewed changes

python/Cargo.toml Show resolved Hide resolved

make native-tls default consistent

1655e28

wjones127 added 12 commits February 11, 2023 08:58

test from top level

f4e483e

format

b1b1933

Need native-tls

0d02e77

what if we made rustls default

419316f

remove unneeded feature

4a59116

switch around feature names

02ac90f

cleanup test commands

0dd3455

fix title

ddefaf3

fix new clippy errors

ce8e92e

just integration test one package at a time

db25376

drop extra feature

929f6eb

simplify path parsing

8e07cc6

wjones127 force-pushed the fix/cargo-check-working branch from 6b566ce to b6bd80b Compare February 11, 2023 18:25

wjones127 commented Feb 11, 2023

View reviewed changes

fix env handling

b99e3be

wjones127 force-pushed the fix/cargo-check-working branch from b6bd80b to b99e3be Compare February 11, 2023 18:43

roeap approved these changes Feb 11, 2023

View reviewed changes

wjones127 merged commit 3ecf0e9 into delta-io:main Feb 11, 2023

wjones127 deleted the fix/cargo-check-working branch February 11, 2023 22:01

roeap mentioned this pull request Feb 17, 2023

Switch back the toolchain to "stable" in the build and test CI jobs #926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rustls default across all packages #1097

Make rustls default across all packages #1097

wjones127 commented Jan 25, 2023

houqp commented Jan 25, 2023

wjones127 Jan 26, 2023

roeap Feb 11, 2023

wjones127 Jan 26, 2023

wjones127 commented Feb 7, 2023

houqp commented Feb 7, 2023

wjones127 commented Feb 11, 2023

wjones127 Feb 11, 2023

roeap Feb 11, 2023

roeap left a comment

roeap Feb 11, 2023

Make rustls default across all packages #1097

Make rustls default across all packages #1097

Conversation

wjones127 commented Jan 25, 2023

Description

Related Issue(s)

Documentation

houqp commented Jan 25, 2023

wjones127 Jan 26, 2023

Choose a reason for hiding this comment

roeap Feb 11, 2023

Choose a reason for hiding this comment

wjones127 Jan 26, 2023

Choose a reason for hiding this comment

wjones127 commented Feb 7, 2023

houqp commented Feb 7, 2023

wjones127 commented Feb 11, 2023

wjones127 Feb 11, 2023

Choose a reason for hiding this comment

roeap Feb 11, 2023

Choose a reason for hiding this comment

roeap left a comment

Choose a reason for hiding this comment

roeap Feb 11, 2023

Choose a reason for hiding this comment