-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix connection parameter handling #190
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
+ Coverage 90.98% 91.05% +0.06%
==========================================
Files 24 24
Lines 5247 5298 +51
==========================================
+ Hits 4774 4824 +50
- Misses 473 474 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
S3Mover is unfortunately not the only place where the connection parameters are used, eg the ScpMover also uses this. It is though not tested, so we can't see it from the CI, but these changes very likely break it.
Also, it feels backwards to pass the full config to the movers when only the connection parameters are needed. Instead, I think we need to modify the move_it_server to only pass the connection parameters to the movers.
Or is there a use case I missed when the full config is needed by the movers?
In Trollmoves Server we've never had the connection parameters, which is a dictionary, because it uses ini configs. Thus all the items are in the main level of the configuration. Putting these various connection related items in a dictionary would need special item-by-item handling when combining them from the config. |
Adjusted based on a short discussion on Slack huddle. |
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.
Looks good! just a couple of comments.
res["connection_parameters"] = {} | ||
for key in original.keys(): | ||
if key in CONNECTION_CONFIG_ITEMS: | ||
res["connection_parameters"][key] = original[key] |
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.
should we have a deprecation warning here? to motivate people to move to the __ syntax?
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.
Hmm. Maybe 🤔 I'll see think about this and see how to document this in the first place.
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.
Warning added in e158f3a
trollmoves/tests/test_movers.py
Outdated
@@ -173,14 +173,14 @@ def test_s3_copy_file_to_base_using_connection_parameters(S3FileSystem): | |||
"""Test copying to base of S3 bucket.""" | |||
# Get the connection parameters: | |||
config = yaml.safe_load(test_yaml_s3_connection_params) | |||
attrs = config['target-s3-example1']['connection_parameters'] | |||
attrs = config['target-s3-example1'] |
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.
shouldn't we be passing only the connection parameters here?
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.
Oh, right. Forgot to revert this part, done in 53a4bc3
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.
LGTM
In #165
S3FileSystem()
were given all the "extra" kwargs given to the class. For Dispatcher this was onlyconnection_parameters
, but for Trollmoves Server this is the full configuration file. The kwarg handling inS3FileSystem
andaiobotocore.core.AioSession
are such that the values not consumed by the former are passed to the latter. This caused unrecognized kwargs from the Trollmoves Server config being passed toAioSession
and as it doesn't have a catch-all**kwargs
they cause it to crashed.This PR changes the config handling in the following way:
trollmoves.movers.move_it()
S3Mover.copy()
gets the dictionaryconnection_parameters
and passes it's items toS3FileSystem()
Note that numeric values given in the nested dictionary are not handled.
S3FileSystem()
#189