Mocking for public_url and put_object

[hugolepetit]

Hello, i am here because i read your nice invitation to contribute into console for Fog Mocks. I explored the project a bit, read about openstack and put this PR together. I don't know how and if i should contribute to test suite. I would appreciate some guidance very much !

Have a nice day !

[houndci-bot]

Unused method argument - container. If it's necessary, use _ or _container as an argument name to indicate that it won't be used. You can also write as put_object() if you want the method to accept any arguments but don't care about them.
Unused method argument - object. If it's necessary, use _ or _object as an argument name to indicate that it won't be used. You can also write as put_object() if you want the method to accept any arguments but don't care about them.
Unused method argument - data. If it's necessary, use _ or _data as an argument name to indicate that it won't be used. You can also write as put_object() if you want the method to accept any arguments but don't care about them.
Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used. You can also write as put_object() if you want the method to accept any arguments but don't care about them.
Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used. You can also write as put_object(*) if you want the method to accept any arguments but don't care about them.

[hugolepetit]

Hello, just a friendly ping in here!

I would very much like to know if someone could review this, or the enhancements needed for it to be considered.

Have a nice day !

[timuralp]

@giglemad thanks for the PR! Have you checked out the other open PR to add mocking support (#262)? I wonder if we could merge these two. I'd be happy to carve out some time to review it.

[hugolepetit]

I did not know about #262 but sure it is huge and interesting. That said, I feel like it tries to do a bit too much at once.

• introduction of a new convention : when needed a Common module is included into Real and Mock => I clearly see some benefits about that, i would love to adopt this convention and document it if people are ok

• openstack_management_url option in initialize, i am not too sure about the rescue introduced in Added mocks for Fog::Storage::OpenStack #262, i would prefer it to blow up. I prefer my implementation on the init code because it is similar to what is done into the real code when you dig further in. The example url i took is from the testing code base

Overall there is a lot of code in #262 that would take quite some time to investigate for me since i dont know the container and large objetc spec at the moment. What i think would be a quick win is investigate the two points i mentioned so i can modify this PR accordingly and help it match with what #262 tries to do before dealing with #262. Ideally a rebase of #262 against this master having #341 merged should not break a thing. What do you think ?

[timuralp]

I'd rather have this merged with some tests that use it -- otherwise the code has a higher chance of bit-rotting. A follow up change could be to introduce in-memory blob storage (similar to what #262) was doing, but agreed that it is a bit daunting to try to do all of what that PR attempted. Thanks for looking into this!

[timuralp]

This looks duplicated from OpenStack::Core.authenticate, which I guess is only included in the Real classes (e.g. in storage/openstack.rb). I looked around and couldn't find a better way to fit this. We could make a mixin that just parses the management URL, but it seems a bit silly. I think this is ok, but if you have an idea on how to avoid duplicating this code, I'd be all ears.

[timuralp]

Could you try enabling some of the tests we currently skip in test/requests to use this with FOG_MOCK?

[timuralp]

Swift returns the etag of the object on PUT. We could mimic similar behavior by returning an MD5 sum of the object. This would allow us to enable some of the requests tests that currently are skipped if FOG_MOCK is set.

[gildub]

Jumping in the middle to highlight and keep in mind the fact that ultimately real code and test code shouldn't be mixed up. The Mock and Real class coupling makes it unnecessarily difficult to evolve. The Mock code can be (and should be) separated. For example: #223.
What do you think?

[timuralp]

@gildub I caught up on the discussion in fog-core and I don't agree with what you're saying. The genesis of the mock bits in lib wasn't actually for testing purposes, but because integrating against fog using stubs was useful in 3rd party applications. For example, this is useful outside of fog tests:

require 'fog/openstack'

Fog.mock!

fog = Fog::DNS::OpenStack::V2.new()
fog.list_zone_transfer_requests.body
fog.create_zone_transfer_request('random-zone-id').body

The pattern is helpful when writing tests in an application that uses fog and do not want to reach out to the remote endpoint every time. It could be even more helpful if some state, like the object name, content, and metadata, was preserved in memory and returned on subsequent requests, allowing for a better test.

I think the way fog uses Mock is misleading where one might immediately conclude it's only for the fog's own tests. I think this is what @geemus was referring to in the discussion in fog/fog-core#198.

[timuralp]

Re-reading more, maybe you're suggesting moving this to lib/mock? That seems fine to me, as long as the behavior such as above is preserved. I think that's outside the scope of this PR, though.

[geemus]

@timuralp yeah, the intention was for end-users to be able to more easily test their usage/integration (not just for usage within fog's own test).

[gildub]

@timuralp, following upon your comment, and especially after #223 recent merge. The feature of mocking fog-openstack is counter-productive and not flexible enough to catch-up with OpenStack evolutions and more precisely Microversions. Although I understand there are use cases for it, keeping it separated in its own plug-in would offer best of both worlds.

[timuralp]

@gildub, reading @plribeiro3000's #223 (comment), @geemus's comment above, your comments, and the conversation in fog/fog-core#198, I don't see all three of you agreeing on a single approach.

Maybe that's not the case and there is agreement? @geemus and @plribeiro3000, do you agree with what @gildub is saying? Earlier today, it seemed @plribeiro3000 had reservations about the recent changes to fog in #223. That's why I don't see a unified plan. I defer to @geemus and @plribeiro3000, but I'd imagine having one fog-* project unlike the others would not be great for fog as a whole.

Regardless of the answer to these questions, I think it would be better to merge this patch without unrelated refactoring changes, especially since the Swift mocks are still in the same place currently. Time-permitting, I can commit to trying to get this PR into a state where it can be merged at the end of this week (unless there is anyone else interested in Swift that can do it sooner/better!).

It has been open a long time and it is unfortunate when contributors lose interest because no one followed through on helping them merge the code. I certainly am guilty of that and would like to try to do better.

[geemus]

@timuralp I would tend to agree that it makes sense for us to separate this particular change and the idea of extracting this to a separate plug-in. We haven't done such an extraction in other providers, but maybe just because they tend not to move as rapidly or change as much. My vote would be to bring this in, if we are satisfied with it as a change, and further the conversation about refactoring/extraction in addition (rather than instead of merging). Thoughts?

[plribeiro3000]

@timuralp You are right. We haven't got to a conclusion on the subject, thus fog/fog-core#198 is still open.
In the wild, this is a big change for the entire fog ecosystem so we must agree on something before we start dropping, changing it. That was the main reason of my concern.

So i agree with @geemus to bring this in and if possible revert #223 because it is a big change to be made and we need to prepare it better (deprecate it, write about the reason on the main fog README, drop it, etc). We need a plan of action.

[gildub]

Follow up discussion: fog/fog-core#198 (comment)

@plribeiro3000, revert would be extreme case scenario, it would be preferable to move it in /lib/mock or so. So that restores full feature with better structure.

[plribeiro3000]

@gildub I understand that but i believe the best approach would be to undo this, and then start working on the move to lib/mock.

That move should happen only after we get to an agreement on fog/fog-core#198. This is important because we need to make sure all providers are in for this change so it wont be as easy as just change in here.

[gildub]

@plribeiro3000, unfortunately I don't see reverting a realistic approach especially in the light the issue being raised 1,5 year ago. The most practical approach is to move it to lib/mock since that would restore the feature in the code and continue going in the right direction.

[plribeiro3000]

@gildub I understand what you are saying about getting it better but as of now we are not doing it correctly.

The issue was raised 1,5 year ago but nothing was concluded from it. As mentioned earlier, this is not a move for you to decide on your own. The whole community need to be onboard of the decision so everyone can do the necessary changes on its own provider. Being it a move to lib/mock or a drop of the current feature this isn't the way to do it.

Until we get to a decision on it it should stay as it was.

Don't get me wrong, i'm not trying to be a PITA but we need to coordinate our efforts or we might break the entire library to the end user. And this means we need to decide before doing any changes like this one.

[gildub]

@plribeiro3000, just because I'm the main interface here doesn't mean I'm the only one behind this.
I'm trying to be practical to get this pain fixed sooner than later. As a matter of fact I proposed a pull request which was accepted.

Anyway, in order to move forward I just added #374 as the move to 'lib/mock' as discussed in fog/fog-core#198 seems the best win-win scenario.

[plribeiro3000]

#374 does not add the feature back as the files are not being required when you require fog-openstack.

Since you are not willing to change this for any reason and i do believe it need to be changed there is no reason to keep this discussion going further. Lets leave it as is for now but please don't keep pushing this forward until we decide how to handle it.

[geemus]

Added some comments on fog-core issue, will leave discussion to there for now to hopefully avoid duplication/confusion.

[timuralp]

I think a lot of the discussion here has been outside of this PR. I prepared it to be in a mergeable state as #375 and going to close this PR, as there have been no response to the review.

[timuralp]

@gildub no problem -- I closed the PR and opened #375, as I wanted to address my own comments, since I haven't heard back from @giglemad in a while.

[gildub]

@timuralp, sorry I realized you just created #375. No worries, thanks.