Split logic into DynamicRemoteOptions.from_option into different code paths #16172
Conversation
… paths Granted, 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.
| oauth_token = ( | ||
| Path(bootstrap_options.remote_oauth_bearer_token_path).resolve().read_text().strip() | ||
| ) | ||
| if set(oauth_token).intersection({"\n", "\r"}): |
There was a problem hiding this comment.
When is there a need to check for '\r'? Why not just if "\n" in oauth_token?
There was a problem hiding this comment.
idk. this code was like this before. I don't thins we need to change this particular check in this PR.
it was added here:
2a5346e#diff-db4651ae839e702898bac44a00db5fd93b21701f624f57479fb537d7bc55a6d1R111
so maybe @Eric-Arellano can provide some context on it.
There was a problem hiding this comment.
I think it was added a long time ago from Daniel Wagner Hall. We should probably keep it.
| token_header = {"authorization": f"Bearer {oauth_token}"} | ||
| execution_headers.update(token_header) | ||
| store_headers.update(token_header) | ||
| execution = cast(bool, bootstrap_options.remote_execution) |
There was a problem hiding this comment.
These casts are duplicative of the casts in the OAuth bearer token function. Maybe have a common function to extract to an intermediate dataclass?
There was a problem hiding this comment.
As mentioned in the PR description, I think having some code duplication is ok.
it makes it easier to understand and maintain, the duplicated code is trivial.
I don't see a real benefit that we would get by introducing a data class to reduce the duplicated code.
There was a problem hiding this comment.
In general I'd agree with Tom, but I think that it's maybe not necessary here since we're constructing a dataclass which provides the type safety of not being able to forget an option.
|
Ping |
| token_header = {"authorization": f"Bearer {oauth_token}"} | ||
| execution_headers.update(token_header) | ||
| store_headers.update(token_header) | ||
| execution = cast(bool, bootstrap_options.remote_execution) |
There was a problem hiding this comment.
In general I'd agree with Tom, but I think that it's maybe not necessary here since we're constructing a dataclass which provides the type safety of not being able to forget an option.
… 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]
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.
context for this change: I am planning to add entry point support for the auth plugin 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.
not adding a label for now in order to prevent CI from running. will do that after the two PRs this change is on top of lands.