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
Add entry point support for remote auth plugin #16212
Conversation
34ee8ef
to
b44e02d
Compare
… paths (#16172) I am planning to [add entry point support for the auth plugin](#16212) so the user doesn't have to specify the plugin path. This will require some changes to the logic here and having a function that deals with loading the plugin (not mixed with the other code paths) will make it simpler and easier to grok. This duplicated some code (extracting options and passing them DynamicRemoteOptions) but I think having 3 code paths for the different use cases justifies that and makes this code more readable and hence more maintainable over time. [ci skip-build-wheels] [ci skip-rust]
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.
Thanks!
plugin_name = ( | ||
auth_plugin_result.plugin_name | ||
or bootstrap_options.remote_auth_plugin | ||
or remote_auth_plugin_func.__name__ |
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.
Won't __name__
always be remote_auth
, since it is required to be for loading? You'll probably want to use the module here instead.
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.
updated
@@ -278,6 +290,7 @@ def from_options( | |||
full_options: Options, | |||
env: CompleteEnvironment, | |||
prior_result: AuthPluginResult | None = None, | |||
remote_auth_plugin_func: Callable | None = 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.
Having a fully specified Protocol
for this type would be good.
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 tried to do it, but this feels like a rabbit hole which is out of scope for this PR.
since arguments are named, if I do:
AuthPluginFunctionProtocol = Callable[[Dict[str, str], Dict[str, str], Options, Mapping[str, str], Union[AuthPluginResult, None]], AuthPluginResult]
and use that instead of callable, mypy complains:
src/python/pants/option/global_options.py:391:30: error: Unexpected keyword
argument "initial_execution_headers" [call-arg]
auth_plugin_result = remote_auth_plugin_func(
^
src/python/pants/option/global_options.py:391:30: error: Unexpected keyword
argument "initial_store_headers" [call-arg]
auth_plugin_result = remote_auth_plugin_func(
^
src/python/pants/option/global_options.py:391:30: error: Unexpected keyword
argument "options" [call-arg]
auth_plugin_result = remote_auth_plugin_func(
^
src/python/pants/option/global_options.py:391:30: error: Unexpected keyword
argument "env" [call-arg]
auth_plugin_result = remote_auth_plugin_func(
^
src/python/pants/option/global_options.py:391:30: error: Unexpected keyword
argument "prior_result" [call-arg]
auth_plugin_result = remote_auth_plugin_func(
^
Found 5 errors in 1 file (checked 29 source files)
so this requires usage of NamedArg which is available only in mypy_extensions currently not used in the pants repo.
beside all that, since this is runtime behavior thing, any type checks here are somewhat pointless since the auth plugin function is loaded at runtime and any validation mypy does in the pants repo itself feels pointless and I don't see what kind of errors the additional and more accurate typing information will prevent.
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.
Sorry, I meant literally Protocol
: https://mypy.readthedocs.io/en/stable/protocols.html#simple-user-defined-protocols
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.
Not sure how to use here since the function is implemented as a function and not a class and from a quick read of the docs you linked to, it seems that Protocol is intended to be use in class based interfaces.
At any case, as I said, this still feels out of scope for this PR (we don't have it with the current implementation) and I don't see the benefit (since this is a runtime behavior) what kind of bugs do you think this kind of annotation will prevent ?
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.
To make a Protocol
for a Callable
you define the __call__
method: an example:
pants/src/python/pants/pantsd/pants_daemon_core.py
Lines 28 to 34 in 258ab26
class PantsServicesConstructor(Protocol): | |
def __call__( | |
self, | |
bootstrap_options: OptionValueContainer, | |
graph_scheduler: GraphScheduler, | |
) -> PantsServices: | |
... |
But yea, as you said: we don't define this API yet, so I'm fine with it being out of scope.
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.
got it, I'll have a follow up PR for this.
since if we this type is exposed from pants, then the plugin implementation can use it.
… paths (pantsbuild#16172) I am planning to [add entry point support for the auth plugin](pantsbuild#16212) so the user doesn't have to specify the plugin path. This will require some changes to the logic here and having a function that deals with loading the plugin (not mixed with the other code paths) will make it simpler and easier to grok. This duplicated some code (extracting options and passing them DynamicRemoteOptions) but I think having 3 code paths for the different use cases justifies that and makes this code more readable and hence more maintainable over time. [ci skip-build-wheels] [ci skip-rust]
Follow up for pantsbuild#16212 This is useful when the remote auth plugin is a first party (in repo plugin). also add some debug logging.
Follow up for #16212 This is useful when the remote auth plugin is a first party (in repo plugin). also add some debug logging. [ci skip-rust]
Follow up for pantsbuild#16212
…ld#16444) Follow up for pantsbuild#16212 This is useful when the remote auth plugin is a first party (in repo plugin). also add some debug logging. [ci skip-rust]
…w. (pantsbuild#16691) Follow up for pantsbuild#16212 [ci skip-rust]
This will remove the need to specify the remote_auth_plugin in the pants config and will allow the auth plugin to be auto discovered using the entry point mechanism.
Once the updated Toolchain Pants Plugin with the entry point support is released and updated in this repo I will mark the
[GLOBAL].remote_auth_plugin
as deprecated.not adding a label for now in order to prevent CI from running. will do that after the PR this change is on top of lands.