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

Fix https://github.com/jubos/fake-s3/issues/114 #3

Closed
wants to merge 1 commit into from

Conversation

Kureev
Copy link

@Kureev Kureev commented Oct 28, 2016

Our fork of fake_s3 can't recognize a request host and compose a 404 response (because we have a different network in docker).

see jubos#114 thread for more details

@aseyedmehdi
Copy link

Is this tested / will it work with our current dev setup still?

@Kureev
Copy link
Author

Kureev commented Oct 28, 2016

  • it's not tested, because the only way to test it is to merge it in and use it
  • this fix has been applied in the origin docker/fake_s3 image
  • current behavior will keep working

@felixhageloh
Copy link

should we merge our fork with upstream if it has been fixed there, instead?

@Kureev
Copy link
Author

Kureev commented Oct 28, 2016

I remember @suweller said that we have something specific that is not a part of the upstream. That's the only reason why we use it. If that's no longer a point, I can use a fake_s3 docker image instead

@felixhageloh
Copy link

we do which is why I said merge with upstream (not replace with upstream)

@felixhageloh
Copy link

this is why we don't use the vanilla gem btw jubos#119

@felixhageloh
Copy link

we do have merge conflicts tho :/

@Kureev
Copy link
Author

Kureev commented Oct 28, 2016

I think for now this particular fix is the leanest iteration

@felixhageloh
Copy link

so to give more detail: fakes3 doesn't / didn't support List Parts which is why we I made a fork (and a PR).
If the latest version does support it we can switch to that again

@felixhageloh
Copy link

not sure it is the leanest:
we should probably stay up to date with upstream so adding more changes will just make the merge conflict worse.
What might be leaner maybe is to not support fakes3 in the first iteration of docker?

@Kureev
Copy link
Author

Kureev commented Oct 28, 2016

we should probably stay up to date with upstream so adding more changes will just make the merge conflict worse.

This change mimics upstream

@felixhageloh
Copy link

felixhageloh commented Oct 28, 2016

#4

maybe see if this branch works for you @Kureev
If it does, maybe rebasing will be better

@suweller
Copy link

Just rebase with upstream

@Kureev
Copy link
Author

Kureev commented Oct 31, 2016

I think we need to communicate it all together

@Kureev
Copy link
Author

Kureev commented Oct 31, 2016

I'm fine with either of these solutions. However, I'm not sure if rebasing the whole lib is a small step. We have 2k+ diff for a one-line fix

@Kureev Kureev closed this Nov 4, 2016
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.

4 participants