Updated value of anon to be False#540
Conversation
kyleknap
left a comment
kyleknap
left a comment
There was a problem hiding this comment.
Looks good! I just had some smaller feedback.
| @@ -315,25 +312,11 @@ def __init__( | |||
| if anon is not None: | |||
There was a problem hiding this comment.
Is there a particular reason, we did not just change the default of the os.getenv() call here to "false"? It would help keep the diff simpler here I think.
CHANGELOG.md
Outdated
| Unreleased | ||
| ---------- | ||
| - Added `**kwargs` to `AzureBlobFileSystem.exists()` | ||
| - **Breaking:** Adlfs now requires credentials as the default value for `anon` has changed to |
There was a problem hiding this comment.
For this changelog, let's:
- Hoist this entry to be top entry in this section since it will be the most important update
- Update the wording to the following to help make what's breaking a little more clear:
**Breaking:** Adlfs no longer uses anonymous authentication by default. It now defaults to using default Azure credentials for authentication. To continue using anonymous authentication please set `anon=True` when creating `AzureBlobFileSystem`.
README.md
Outdated
| The value to use for whether to attempt anonymous access if no other credential is passed. By default (`None`), the | ||
| `AZURE_STORAGE_ANON` environment variable is checked. False values (`false`, `0`, `f`) will resolve to `False` and | ||
| anonymous access will not be attempted. Otherwise the value for `anon` resolves to True. | ||
| `AZURE_STORAGE_ANON` environment variable is checked. If the environment variable is set, values of (``false``, ``0``, ``f``) are ``False`` and everything else resolves to True. Anon will default to ``False`` if nothing | ||
| is provided and ``DefaultAzureCredential`` will be used for authentication. |
There was a problem hiding this comment.
I think we just narrow down the description here to just:
Set to `True` to use anonymous authentication. If not set, the `AZURE_STORAGE_ANON` environment variable will also be checked.
Mainly I'm not sure if we need to go into all of the details here as it makes the description sort of hard to follow.
README.md
Outdated
There was a problem hiding this comment.
At the very beginning here, it would be worth calling out that by default if no additional credentials/configuration are provided, DefaultAzureCredentials will be used so that is clear before we go into detail in the storage_options what you get if you don't configure any of this.
| and self.account_key is None | ||
| and self.sas_token is None | ||
| and self.client_id is not None | ||
| and self.connection_string is None |
There was a problem hiding this comment.
Was there a particular reason for adding connection_string check here? More just curious if there was something specific that you ran into or if this was more for correctness.
There was a problem hiding this comment.
Some test in test_connect_client was failing because it was using the connection string and since anon would now be false it was still trying to get the credentials.
adlfs/tests/test_spec.py
Outdated
| ], | ||
| ) | ||
| def test_no_anon_warning(storage, env_vars, storage_options): | ||
| def test_anon_default(storage, env_vars, storage_options): |
There was a problem hiding this comment.
Let's rename the test to test_anon_off as we are still configuring anon in some cases and this is more to make sure that anonymous connection is turned off in these cases.
| skip_instance_cache=True, | ||
| **storage_options, | ||
| ) | ||
| assert fs.anon is False |
There was a problem hiding this comment.
It would also be worth adding an assertion similar to what we have in test_connect_client.py to detect whether we are actually providing credentials to the client. Mainly, while the fs.anon could be set to one value, there could be unintended behavior in the code where the credential was not actually passed to the client. I don't think we need to assert the exact credentials provided either it would be more if credential was set to any non-None value when instantiating the client.
2 participants
We are updating the default value of
anonto beFalsewhich means that anonymous authentication will not be used unlessanon=Trueis explicitly passed in when instantiatingAzureBlobFileSystem.DefaultAzureCredentialswill now become the default authentication method. This resolves #348.