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

Initial work for split out of s3control #4714

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SimonToftegaard
Copy link
Contributor

As discussed in #4707 here is the initial separate implementation of s3control

@bblommers
Copy link
Collaborator

Thanks @SimonToftegaard

@bblommers bblommers added this to the 3.0.0 milestone Dec 22, 2021
@bblommers bblommers marked this pull request as draft December 22, 2021 20:31
@SimonToftegaard
Copy link
Contributor Author

Seems like the unit tests in server mode do not get intercepted correctly by the mock_s3control, not sure how to fix this.

@bblommers
Copy link
Collaborator

One potential cause could be that the request is picked up by the S3-module, instead of S3control, because the regex for the S3-urls will still catch requests to s3control-urls. That's just a hunch though, I'd have look a little deeper into it to verify whether that's the case

@bblommers
Copy link
Collaborator

OK, found the issue - ServerMode tests will always fail for S3Control.
From the original tests:

# All tests for s3-control cannot be run under the server without a modification of the
# hosts file on your system. This is due to the fact that the URL to the host is in the form of:
# ACCOUNT_ID.s3-control.amazonaws.com <-- That Account ID part is the problem. If you want to
# make use of the moto server, update your hosts file for `THE_ACCOUNT_ID_FOR_MOTO.localhost`
# and this will work fine.

#4745 contains a clean implementation of this change, without any changes to the logic or tests. We will merge that one first, to ensure we're not breaking anything else.
After that we can have a look at extending the logic to allow other account ID's.

@bblommers bblommers removed this from the 3.0.0 milestone Jan 25, 2022
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.

2 participants