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

Support the latest version of botocore #1847

Closed
wants to merge 2 commits into from

Conversation

joguSD
Copy link
Contributor

@joguSD joguSD commented Oct 2, 2018

As discussed in #1793 in a recent version of botocore, the vendored version of requests was dropped in favor of a direct dependency on urllib3. This broke the MockAWS implementation based on responses. As of botocore version 1.12.13 the before-send event has been added which allows for the HTTP layer in botocore to be bypassed by registering a handler to this event that will introspect the request and return the appropriate mocked response.

The event handler implementation is largely the same as the logic used in the responses module but boiled down to the functionality required by moto. This event handler is registered globally at the import time of moto and all botocore clients created while moto is imported will be able to be mocked.

Unfortunately, the before-send event cannot support all of the functionality that mocking out requests could. Specifically, it cannot support mocking the instance metadata credential resolver. This means that credentials need to be provided to botocore in some manner now for the mocks to function, which is why I've set the credential environment variables to be present for all tests. Currently the tests that fail are either expecting instance metadata to be mocked out when accessed via requests, or are otherwise using requests. However, this PR should support the majority of moto use cases.

@coveralls
Copy link

Coverage Status

Coverage decreased (-28.7%) to 63.895% when pulling 2ece167 on joguSD:botocore-event into dfa7935 on spulec:master.

@spulec
Copy link
Collaborator

spulec commented Oct 15, 2018

This is looking great; thanks for all of your work on this.

If we can add joguSD#1 on top of this, I think we are in a good place to merge and hopefully push past all of this.

If I get some time in the next couple of days, I'll try to get the final parts of this done. If anyone else gets some time before that, feel free to jump in.

@joguSD
Copy link
Contributor Author

joguSD commented Oct 15, 2018

@spulec Do you think continuing to patch requests is the correct approach to get all the tests working again? botocore no longer uses requests anywhere so it's really just changing how the current stubber implementation works just to get the tests that use requests directly to pass. In practice, requests will never be used by botocore so I guess it comes down to a question of whether or not you view:

@mock_s3
def main():
    requests.get('s3.url') # should be mocked

as a feature of moto and not just a byproduct of the implementation.

@spulec
Copy link
Collaborator

spulec commented Oct 16, 2018

I think that is a nice feature of Moto (at least that is how we have made it work previously) so I am in favor of that.

@jakul
Copy link

jakul commented Oct 16, 2018

I am -1 on still using responses in moto, because it interferes with my use of responses as an application developer. I raised an issue about this previously #1598 for which I don't currently have a reasonable workaround (other than still using moto 1.2). Removing responses completely from Moto will definitely make my life simpler!

@lhufnagel
Copy link

Yeah, actually I agree to that

@spulec
Copy link
Collaborator

spulec commented Oct 17, 2018

I suppose we could have a flag to allow the user to control if we setup responses mocking.

@e271828-
Copy link

Look forward to seeing this work land in a release. May be worth updating the README at the top to warn people in the interim. Also -1 on keeping responses around.

@joguSD
Copy link
Contributor Author

joguSD commented Oct 22, 2018

@spulec Any updates on this? I think in the short-term just getting moto working with newer versions of botocore is important, and the decision to add configuration for responses or removing it can be a separate issue.

@spulec
Copy link
Collaborator

spulec commented Oct 28, 2018

Agreed that we can discuss if responses should be removed later, since we can't remove it now and break existing behavior.

I don't have a lot of time right now, but if someone has time to create a PR that works with all of our existing tests, I'll be happy to merge and cut a new release.

@lhufnagel
Copy link

lhufnagel commented Oct 30, 2018

@spulec can you have a look at this #1907? Thx!

@spulec
Copy link
Collaborator

spulec commented Oct 31, 2018

@lhufnagel the code looks good!

Can someone else here give the branch in #1907 a test? Due to the nature of this change, would be great to have another person or two test it.

@jvtm
Copy link

jvtm commented Oct 31, 2018

I was able to run our internal project's test cases with:

  • Boto versions from pip freeze:

      boto==2.49.0
      boto3==1.9.34
      botocore==1.12.34
    
  • Get all tests working with latest botocore  #1907 checkout in PYTHONPATH

  • BOTO_CONFIG=/dev/null AWS_SECRET_ACCESS_KEY=bogus AWS_ACCESS_KEY_ID=bogus PYTHONPATH=$HOME/checkout/moto pytest ...

  • tests use mock_s3() and mock_sqs() in few different ways (fixtures for pytest, setup/teardown for traditional unittest)

  • verified manually that I don't have working creds in this environment, and even run the relevant parts of the test set without network :)

👍

@rouge8
Copy link
Contributor

rouge8 commented Nov 2, 2018

#1907 passes on one of my Python projects, using cloudformation, ec2, elb, iam, s3, rds, route53, and sqs mocks.

@spulec
Copy link
Collaborator

spulec commented Nov 4, 2018

Closing as these changes were merge with #1907

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants