feat: add custom Azure host/port to support custom blob endpoint #164

jeqo · 2023-12-20T15:31:52Z

About this change - What it does

Adds custom host and port configs to Azure to support custom blob endpoint like Azurite.

Resolves: #163

Why this way

Follow similar approach to S3, to set custom host/port.
Moved conn_string construction out of the initialization to be able to test it.

codecov-commenter · 2023-12-20T16:16:24Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (f3b1f53) 71.97% compared to head (e5e4303) 72.44%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
rohmu/object_storage/azure.py	76.92%	3 Missing ⚠️
rohmu/object_storage/config.py	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   71.97%   72.44%   +0.47%     
==========================================
  Files          35       35              
  Lines        3928     4035     +107     
==========================================
+ Hits         2827     2923      +96     
- Misses       1101     1112      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rohmu/object_storage/azure.py

test/object_storage/test_azure.py

exg

Fixup commits should be folded.

giacomo-alzetta-aiven · 2024-01-02T08:06:57Z

Overall LGTM but I agree with Emanuele that commit history could be cleaned up before merging.

jeqo · 2024-01-02T11:32:50Z

@exg @giacomo-alzetta-aiven thanks! Commits are folded now, have another look.

exg · 2024-01-02T12:46:11Z

rohmu/object_storage/azure.py

+            conn.append(f"EndpointSuffix={endpoint_suffix}")
+        else:
+            if not host or not port:
+                raise InvalidConfigurationError("Custom host and port must be specified together")


It would be better to use pydantic validators for custom validation, see https://docs.pydantic.dev/1.10/usage/validators/

Good idea. Adding this on the latest commit, have a look

The validation of azure_cloud can also be moved in the validator. Also, remember to fold fixup commits before pushing :)

Sure, can move it to validators as well.

Re: fixups, I prefer to keep them separate for easier review, but can surely rebase/squash once approved if that's ok.

That is also fine, and I used to recommend that too before. I just find it unnecessary given that GitHub provides the diff between the previous and current branch tip on force push (via the "Compare" button).

Also if a reviewer marks the files as "viewed" when reviewing GitHub will also show them which files have changed when the PR is updated, so it's not like reviewers are forced to re-read everything to discover what you updated.

Thanks, will keep that in mind.
I've squashed all commits now for this change.

exg · 2024-01-02T17:05:19Z

rohmu/object_storage/config.py

+    @root_validator
+    @classmethod
+    def host_and_port_must_be_set_together(cls, values: Dict[str, Any]) -> Dict[str, Any]:
+        if "host" in values and "port" in values:


This test is redundant because the fields have a default value and so they are always present. BTW, another option would be to introduce a HostInfo model, similar to ProxyInfo, containing the new fields. This way there would be no need for a custom validator, and it would be more consistent. WDYT?

Good catch, fixing that on the latest commit.

About HostInfo, I like the idea, but we could do that as a separate refactoring PR as there are many places where this type would fit. At the moment this is consistent with the S3 approach to set custom host/port.

Agreed, I did not realize that this is based on the existing S3 config.

exg · 2024-01-03T07:51:33Z

rohmu/object_storage/config.py

+    @root_validator
+    @classmethod
+    def host_and_port_must_be_set_together(cls, values: Dict[str, Any]) -> Dict[str, Any]:
+        if not values["host"] or not values["port"]:


This rejects (existing) configs where host and port are both missing, which is not correct. I think you want

(values["host"] is None) != (values["port"] is None)

Yeah, test_azure_config_host_port_set_together or a similar test should also verify that the valid cases do not raise an error.

Oh, good catch! I added tests to validate the minimal case and fix this.

e.g. azurite

giacomo-alzetta-aiven

LGTM but I will wait @exg review too before merging

giacomo-alzetta-aiven · 2024-01-03T14:29:27Z

rohmu/object_storage/azure.py

+        if not host and not port:
+            endpoint_suffix = AZURE_ENDPOINT_SUFFIXES[azure_cloud]
+            conn.append(f"EndpointSuffix={endpoint_suffix}")
+        else:
+            conn.append(f"BlobEndpoint={protocol}://{host}:{port}/{account_name}")


nit: I guess we could do something like:

Suggested change

if not host and not port:

endpoint_suffix = AZURE_ENDPOINT_SUFFIXES[azure_cloud]

conn.append(f"EndpointSuffix={endpoint_suffix}")

else:

conn.append(f"BlobEndpoint={protocol}://{host}:{port}/{account_name}")

if not host and not port:

endpoint_suffix = AZURE_ENDPOINT_SUFFIXES[azure_cloud]

conn.append(f"EndpointSuffix={endpoint_suffix}")

elif host and port:

conn.append(f"BlobEndpoint={protocol}://{host}:{port}/{account_name}")

else:

raise ValueError("You must either specify both host and port or neither of them")

In most cases the AzureTransfer will be build using the get_transfer facade where we have the PyDantic validation, but I believe people can still manually create the transfer, so maybe validating this again here is better.

So it is possible to create transfers bypassing the pydantic config models?

I'm just saying that we are not indicating that AzureTransfer is private, so users could instantiate it directly instead of going through the factory methods that use from_model to create the instance from the pydantic configuration.

I don't this is a big deal, hence why I class this as nitpicking.

giacomo-alzetta-aiven · 2024-01-03T18:02:28Z

Since we all agree let's merge this.

jeqo requested a review from giacomo-alzetta-aiven December 20, 2023 15:34

jeqo force-pushed the jeqo/custom-azure-url branch 4 times, most recently from 9a54a02 to c0ce800 Compare December 20, 2023 16:12

jeqo requested a review from rominf December 20, 2023 16:32

rominf suggested changes Dec 21, 2023

View reviewed changes

rohmu/object_storage/azure.py Outdated Show resolved Hide resolved

rohmu/object_storage/azure.py Outdated Show resolved Hide resolved

rohmu/object_storage/azure.py Outdated Show resolved Hide resolved

test/object_storage/test_azure.py Outdated Show resolved Hide resolved

jeqo requested a review from rominf December 21, 2023 12:09

jeqo force-pushed the jeqo/custom-azure-url branch 3 times, most recently from 2f2c3b9 to a7ae503 Compare December 28, 2023 11:02

rominf approved these changes Jan 2, 2024

View reviewed changes

exg requested changes Jan 2, 2024

View reviewed changes

jeqo force-pushed the jeqo/custom-azure-url branch from 8d91072 to 2e7e68f Compare January 2, 2024 10:53

jeqo requested a review from exg January 2, 2024 10:53

exg reviewed Jan 2, 2024

View reviewed changes

jeqo force-pushed the jeqo/custom-azure-url branch 2 times, most recently from f4e5491 to 9ff51d4 Compare January 2, 2024 14:16

jeqo requested a review from exg January 2, 2024 15:05

jeqo force-pushed the jeqo/custom-azure-url branch from 9a3dd93 to aee7dee Compare January 2, 2024 16:55

exg reviewed Jan 2, 2024

View reviewed changes

jeqo force-pushed the jeqo/custom-azure-url branch from aee7dee to e5e4303 Compare January 2, 2024 18:17

jeqo requested a review from exg January 2, 2024 18:19

exg reviewed Jan 3, 2024

View reviewed changes

jeqo force-pushed the jeqo/custom-azure-url branch from e5e4303 to 04fe766 Compare January 3, 2024 12:58

jeqo requested a review from exg January 3, 2024 12:59

feat: add custom Azure host/port to support custom blob endpoint

579d154

e.g. azurite

jeqo force-pushed the jeqo/custom-azure-url branch from 04fe766 to 579d154 Compare January 3, 2024 14:17

giacomo-alzetta-aiven approved these changes Jan 3, 2024

View reviewed changes

exg approved these changes Jan 3, 2024

View reviewed changes

giacomo-alzetta-aiven merged commit 7107456 into main Jan 3, 2024
8 checks passed

giacomo-alzetta-aiven deleted the jeqo/custom-azure-url branch January 3, 2024 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add custom Azure host/port to support custom blob endpoint #164

feat: add custom Azure host/port to support custom blob endpoint #164

jeqo commented Dec 20, 2023 •

edited

Loading

codecov-commenter commented Dec 20, 2023 •

edited

Loading

exg left a comment

giacomo-alzetta-aiven commented Jan 2, 2024

jeqo commented Jan 2, 2024

exg Jan 2, 2024

jeqo Jan 2, 2024

exg Jan 2, 2024

jeqo Jan 2, 2024

exg Jan 2, 2024

giacomo-alzetta-aiven Jan 2, 2024

jeqo Jan 2, 2024

exg Jan 2, 2024

jeqo Jan 2, 2024

exg Jan 3, 2024

exg Jan 3, 2024

giacomo-alzetta-aiven Jan 3, 2024

jeqo Jan 3, 2024

giacomo-alzetta-aiven left a comment

giacomo-alzetta-aiven Jan 3, 2024

exg Jan 3, 2024

giacomo-alzetta-aiven Jan 3, 2024

giacomo-alzetta-aiven commented Jan 3, 2024

feat: add custom Azure host/port to support custom blob endpoint #164

feat: add custom Azure host/port to support custom blob endpoint #164

Conversation

jeqo commented Dec 20, 2023 • edited Loading

About this change - What it does

Why this way

codecov-commenter commented Dec 20, 2023 • edited Loading

Codecov Report

exg left a comment

Choose a reason for hiding this comment

giacomo-alzetta-aiven commented Jan 2, 2024

jeqo commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giacomo-alzetta-aiven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giacomo-alzetta-aiven commented Jan 3, 2024

jeqo commented Dec 20, 2023 •

edited

Loading

codecov-commenter commented Dec 20, 2023 •

edited

Loading