-
Notifications
You must be signed in to change notification settings - Fork 56
feat(azure-storage): Add config parsing from connection string #524
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: DerGut <jannik.steinmann@gmx.de>
|
|
||
| /// The Azure Storage service that a configuration or credential is used with. | ||
| #[derive(PartialEq)] | ||
| pub enum Service { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
|
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. |
Signed-off-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
Signed-off-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
|
@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. |
Closes #522
Public Changes
➕ Adds a
reqsign-azure-storage::Config::try_from_connection_stringfunction to allow parsing Azure Storage connection strings.➕ Adds a
reqsign-azure-storage::Serviceenum that enumerates the different Azure Storage service offeringsRationale
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
Serviceenum 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.