Skip to content

Conversation

@kartikgupta-db
Copy link
Contributor

@kartikgupta-db kartikgupta-db commented Jul 29, 2022

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #526 (0dc8638) into main (44ccc7d) will increase coverage by 3.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
+ Coverage   60.93%   64.79%   +3.86%     
==========================================
  Files          55       55              
  Lines        4669     4727      +58     
==========================================
+ Hits         2845     3063     +218     
+ Misses       1824     1664     -160     
Impacted Files Coverage Δ
databricks_cli/utils.py 89.58% <0.00%> (-0.53%) ⬇️
databricks_cli/unity_catalog/perms_cli.py 0.00% <0.00%> (ø)
databricks_cli/unity_catalog/metastore_cli.py 0.00% <0.00%> (ø)
databricks_cli/unity_catalog/delta_sharing_cli.py 0.00% <0.00%> (ø)
databricks_cli/configure/provider.py 95.97% <0.00%> (+0.09%) ⬆️
databricks_cli/unity_catalog/uc_service.py 27.73% <0.00%> (+27.73%) ⬆️
databricks_cli/unity_catalog/utils.py 30.76% <0.00%> (+30.76%) ⬆️
databricks_cli/unity_catalog/api.py 47.97% <0.00%> (+47.97%) ⬆️
databricks_cli/unity_catalog/lineage_cli.py 80.00% <0.00%> (+80.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kartikgupta-db kartikgupta-db requested a review from fjakobs July 29, 2022 20:08
Copy link
Contributor

@fjakobs fjakobs left a comment

Choose a reason for hiding this comment

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

Fix looks reasonable but I'd like to get @pietern's opinion before merging since it would be a breaking change.

@pietern
Copy link
Contributor

pietern commented Aug 12, 2022

We can't merge this as is. Reading through the linked issues, there's an opportunity to do this in a non-breaking way. The default auth handler checks the host and overrides a token from .netrc if present, regardless of existing authentication being configured. What we can do instead is fallback to reading from .netrc if nothing is configured. Then the case where a user has a .netrc and only specifies a hostname, the CLI will continue to work.

@pietern
Copy link
Contributor

pietern commented Aug 12, 2022

This is a nice workaround. In the body we could check for the Authorization header and only defer to .netrc if it isn't set.

psf/requests#2773 (comment)

@kartikgupta-db
Copy link
Contributor Author

We can't merge this as is. Reading through the linked issues, there's an opportunity to do this in a non-breaking way. The default auth handler checks the host and overrides a token from .netrc if present, regardless of existing authentication being configured. What we can do instead is fallback to reading from .netrc if nothing is configured. Then the case where a user has a .netrc and only specifies a hostname, the CLI will continue to work.

The cli checks that there should be atleast 1 form of accepted configuration methods (spark config, environment variables or databrickscfg) are present. So there should be no cli user directly dependent on a .netrc.

For users using the api client directly from the sdk, the fallback should provide them the similar experience as they have now, with the caveat that now other forms of configurations will take precedence (earlier .netrc had the highest precedence). But this is an undocumented use case.

@kartikgupta-db kartikgupta-db requested a review from pietern August 17, 2022 15:52
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Please add a couple tests:

  • .netrc exists with a valid entry, but auth is specified elsewhere.
  • .netrc exists with a valid entry, auth is not specified.
  • .netrc exists with an invalid entry
  • .netrc does not exist

As is, I'm worried what would happen all cases but the first.

@pietern
Copy link
Contributor

pietern commented Aug 18, 2022

For users using the api client directly from the sdk, the fallback should provide them the similar experience as they have now, with the caveat that now other forms of configurations will take precedence (earlier .netrc had the highest precedence). But this is an undocumented use case.

It's one we need to call out in the changelog / release notes though ;-)

@kartikgupta-db kartikgupta-db requested a review from pietern August 18, 2022 10:21
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

What happens if you have a netrc file with garbage / invalid content?

I expect the default behavior in the requests library is to ignore it. Can you confirm this implementation does the same?

@kartikgupta-db kartikgupta-db requested a review from pietern August 18, 2022 11:16
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@pietern pietern changed the title Disable .netrc conf for api client Don't let .netrc take precedence in ApiClient Aug 18, 2022
@pietern pietern merged commit 9d8e829 into databricks:main Aug 18, 2022
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.

4 participants