-
Notifications
You must be signed in to change notification settings - Fork 989
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
minor improvements to auth plugins #17041
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,11 +33,11 @@ class _SourceURLCredentials: | |
""" | ||
def __init__(self, cache_folder): | ||
self._urls = {} | ||
self.auth_source_plugin = None | ||
self._auth_source_plugin = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer private by default |
||
if not cache_folder: | ||
return | ||
auth_source_plugin_path = HomePaths(cache_folder).auth_source_plugin_path | ||
self.auth_source_plugin = _load_auth_source_plugin(auth_source_plugin_path) | ||
self._auth_source_plugin = _load_auth_source_plugin(auth_source_plugin_path) | ||
creds_path = os.path.join(cache_folder, "source_credentials.json") | ||
if not os.path.exists(creds_path): | ||
return | ||
|
@@ -68,9 +68,9 @@ def _get_auth(credentials): | |
|
||
def add_auth(self, url, kwargs): | ||
# First, try to use "auth_source_plugin" | ||
if self.auth_source_plugin: | ||
if self._auth_source_plugin: | ||
try: | ||
c = self.auth_source_plugin(url) | ||
c = self._auth_source_plugin(url) | ||
except Exception as e: | ||
msg = f"Error while processing 'auth_source_remote.py' plugin" | ||
msg = scoped_traceback(msg, e, scope="/extensions/plugins") | ||
|
@@ -206,8 +206,8 @@ def _call_method(self, method, url, **kwargs): | |
os.environ.clear() | ||
os.environ.update(old_env) | ||
|
||
|
||
def _load_auth_source_plugin(auth_source_plugin_path): | ||
if os.path.exists(auth_source_plugin_path): | ||
mod, _ = load_python_file(auth_source_plugin_path) | ||
if hasattr(mod, "auth_source_plugin"): | ||
return mod.auth_source_plugin | ||
return getattr(mod, "auth_source_plugin", None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More pythonic, single instruction, better than check-then-return |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,11 +12,13 @@ | |
from conans.errors import ConanException, scoped_traceback | ||
from conans.util.files import load | ||
|
||
|
||
class RemoteCredentials: | ||
def __init__(self, cache_folder, global_conf): | ||
self._global_conf = global_conf | ||
self._urls = {} | ||
self.auth_remote_plugin = _load_auth_remote_plugin(HomePaths(cache_folder).auth_remote_plugin_path) | ||
auth_plugin_path = HomePaths(cache_folder).auth_remote_plugin_path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid too long lines |
||
self._auth_remote_plugin = _load_auth_remote_plugin(auth_plugin_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Private by default |
||
creds_path = os.path.join(cache_folder, "credentials.json") | ||
if not os.path.exists(creds_path): | ||
return | ||
|
@@ -31,15 +33,11 @@ def __init__(self, cache_folder, global_conf): | |
except Exception as e: | ||
raise ConanException(f"Error loading 'credentials.json' {creds_path}: {repr(e)}") | ||
|
||
def auth(self, remote, user=None, password=None): | ||
if user is not None and password is not None: | ||
return user, password | ||
|
||
def auth(self, remote, user=None): | ||
# First get the auth_remote_plugin | ||
|
||
if self.auth_remote_plugin is not None: | ||
if self._auth_remote_plugin is not None: | ||
try: | ||
plugin_user, plugin_password = self.auth_remote_plugin(remote, user=user) | ||
plugin_user, plugin_password = self._auth_remote_plugin(remote, user=user) | ||
except Exception as e: | ||
msg = f"Error while processing 'auth_remote.py' plugin" | ||
msg = scoped_traceback(msg, e, scope="/extensions/plugins") | ||
|
@@ -82,8 +80,8 @@ def _get_env(remote, user): | |
ConanOutput().info("Got password '******' from environment") | ||
return user, passwd | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pep8 says 2 blank lines here (enabling some linter in your IDE will make this highlighted) |
||
def _load_auth_remote_plugin(auth_remote_plugin_path): | ||
if os.path.exists(auth_remote_plugin_path): | ||
mod, _ = load_python_file(auth_remote_plugin_path) | ||
if hasattr(mod, "auth_remote_plugin"): | ||
return mod.auth_remote_plugin | ||
return getattr(mod, "auth_remote_plugin", None) |
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.
This is the main change. I removed the
password
argument in thecreds.auth()
, it was confusing and it made you add it in your PR, because I think the argument was not right in the first place