Skip to content
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 config argument to boto3 resource (closes #168) #325

Merged
merged 2 commits into from
Nov 13, 2015

Conversation

mbarrien
Copy link
Contributor

This pull request adds a config argument to the boto3.resource()/boto3.session.resource() function. This allows passing in a custom config to programatically set a signature version, an s3 addressing style, or any other argument from the botocore client config.

It was done this way over passing a client argument in (as suggested in #168) because it prevents the user from passing in a client of the wrong service type, as illustrated in this hypothetical set of calls.

client = boto3.client('sqs', config=Config())
resource = boto3.resource('ec2', client=client)

(You could make it so that resource ignores the service_name argument passed into it if client is specified, but that's really confusing since service_name is a required argument of boto3.resource().)

The only other non-straightforward thing in this pull request that would be nice to have feedback on is the copy.deepcopy performed if the config has not passed in user_agent_extra. This was done so that the incoming argument is not modified, but another possible semantic is that we use user_agent_extra untouched if a config is passed in, not attempting to override with a default value. Any preference, or okay as is?

@mbarrien
Copy link
Contributor Author

mbarrien commented Nov 3, 2015

Ping?

@kyleknap
Copy link
Member

kyleknap commented Nov 3, 2015

I will take a look at it this week. We definitely need a configuration functionality like this in the resource interface. I think accepting the Config object is the way we should go. Another idea that we were considering was the ability to pass in a client to the resource.

@kyleknap kyleknap added the pr/needs-review This PR needs a review from a member of the team. label Nov 3, 2015
@mbarrien
Copy link
Contributor Author

mbarrien commented Nov 3, 2015

Note that the description of this pull request above did explicitly discuss a problem with passing a client to the resource. May not block that idea, but it definitely points out how error prone it could be.

@kyleknap
Copy link
Member

kyleknap commented Nov 3, 2015

Those were good points. Seems like there are a lot of edge cases in being able to pass in a client. I think sticking with the config is the right direction for now.

@jamesls
Copy link
Member

jamesls commented Nov 4, 2015

I agree, 👍 to preferring configs over clients, @mbarrien makes a good case for this.

@@ -268,7 +277,12 @@ def resource(self, service_name, region_name=None, api_version=None,
# and service model, the resource version and resource JSON data.
# We pass these to the factory and get back a class, which is
# instantiated on top of the low-level client.
config = Config(user_agent_extra='Resource')
Copy link
Member

Choose a reason for hiding this comment

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

if config is not None

@jamesls
Copy link
Member

jamesls commented Nov 11, 2015

:shipit: Code looks good to me. Thanks for the pull request!

@mbarrien
Copy link
Contributor Author

Updated per comment. You're okay with the deepcopy being used here?

@themanifold themanifold mentioned this pull request Nov 13, 2015
@jamesls
Copy link
Member

jamesls commented Nov 13, 2015

@mbarrien I think that's fine. Thanks for the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/needs-review This PR needs a review from a member of the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants