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

Add connection Credentials to StorageLocation #743

Closed
wants to merge 5 commits into from

Conversation

lizardoluis
Copy link
Collaborator

@lizardoluis lizardoluis commented Nov 21, 2024

This PR adds the field credentials to the StorageLocationInfo. The credentials will be stored using user mappings, therefore I needed to separate them from options. Other changes include renaming the struct in clade StorageLocation::location to StorageLocation::url, which matches the naming we are using throughout the system.

@lizardoluis lizardoluis self-assigned this Nov 21, 2024
@lizardoluis lizardoluis force-pushed the storage-location-info branch from a4074f5 to af4821f Compare November 21, 2024 18:17
@lizardoluis lizardoluis force-pushed the storage-location-info branch from a27d7aa to 91ffb6e Compare November 21, 2024 21:24
@lizardoluis lizardoluis marked this pull request as ready for review November 22, 2024 08:13
@lizardoluis lizardoluis requested review from gruuya, mildbyte and SergeiPatiakin and removed request for mildbyte November 22, 2024 08:13
Copy link
Contributor

@gruuya gruuya left a comment

Choose a reason for hiding this comment

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

I'm not sure I follow why we need to separate out the credentials in the first place? In this way we add some friction to the API, with one having to delineate what is an option and what is a credential (whereas downstream crates, such as object_store and iceberg rust storage treat them the same).

Since they end up in the same place anyway why just not simply merge the two maps prior to calling build_object_store_from_opts/S3config::from_hashmap and leave the object_store_factory as is?

Comment on lines +265 to 268
options.insert(
"google_service_account".to_string(),
"my_google_service_account".to_string(),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be in credentials now?

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.

2 participants