Skip to content

Conversation

@DerGut
Copy link

@DerGut DerGut commented Jun 9, 2025

Closes #522

Public Changes

➕ Adds a reqsign-azure-storage::Config::try_from_connection_string function to allow parsing Azure Storage connection strings.
➕ Adds a reqsign-azure-storage::Service enum that enumerates the different Azure Storage service offerings

Rationale

Really, this is explained in the issue #522. It's mostly moving code added to OpenDAL with apache/opendal#6212 down into reqsign.
The reason why I added a Service enum is that parsing looks at different values depending on the service used. It also builds a different endpoint string depending on the service.

Tests

Added unit tests for connection_string::parse.

Signed-off-by: DerGut <jannik.steinmann@gmx.de>
@DerGut DerGut marked this pull request as ready for review June 9, 2025 21:29

/// The Azure Storage service that a configuration or credential is used with.
#[derive(PartialEq)]
pub enum Service {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's good idea we just accept plain string?

Copy link
Author

Choose a reason for hiding this comment

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

We certainly can! What do you think would be improved by taking a string? Is it primarily less verbose code in reqsign, or a better UX for try_from_connection_string?

Regarding try_from_connection_string's UX: I could see Service implement FromStr to allow for more dynamic use cases (e.g. picking the right service from a parsed config, etc.). Of course, it would still be more verbose than using a plain string.

I think if we know that this will stay the only place that uses a Service, there wouldn't be much value in the added complexity. But the issue #525 proposes another function that would take a Service.
IMO the more a concept (like an Azure Storage Service) is used, the more it justifies introducing a dedicated type, simply because it serves as a great place for documentation and shows how the same concept is used across a codebase.

Maybe 1-2 uses isn't yet clear enough evidence to make this call. At this point it becomes personal preference. My preference tends to be to introduce a type when unclear (in public APIs) because adding one later is much harder.

But I'm happy to take a string as well!

Copy link
Member

Choose a reason for hiding this comment

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

I actually don't want to expose such API. Maybe a public constant would be sufficient?

Copy link
Author

Choose a reason for hiding this comment

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

That's fair! Will make the change 👍

@Xuanwo
Copy link
Member

Xuanwo commented Jun 17, 2025

Thank you @DerGut for your work on this! I have finished the long-overdue refactor of reqsign, and it's now ready for merging PRs.

DerGut added 2 commits June 17, 2025 12:37
Signed-off-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
Signed-off-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
@tisonkun
Copy link
Member

tisonkun commented Oct 4, 2025

@DerGut Sorry that the refactor change the file structure totally. Would you try to redo the patch on the latest codebase? Maybe separate them into small parts so that we can move fast.

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.

Add Azure Storage Config parsing from a connection string

3 participants