data pipeline mocks #318
data pipeline mocks #318
Conversation
|
Broadly looks good to me (I have no experience with pipeline stuff though). @lanej I'll leave it to you? Thanks. |
|
@ehowe ready ? |
|
Yup, let's do it. On Wed, Nov 16, 2016, 6:49 PM Josh Lane notifications@github.com wrote:
|
| 'pipelineObjects' => transform_objects(pipeline_objects), | ||
| }.merge(options) | ||
|
|
||
| response = request({ |
There was a problem hiding this comment.
nitpick: unnecessary assignment. couple of these lying around.
| }) | ||
|
|
||
| Fog::JSON.decode(response.body) |
There was a problem hiding this comment.
We are changing the return value for all of these requests to the raw response from the decoded body. While I think this is good direction, I would like @fog/fog-aws to comment on the relevance of the breaking change.
There was a problem hiding this comment.
Definitely brings it in line with the rest of the services. Without making that change, it makes it impossible to assert on things that return an empty body, but different status codes.
|
@geemus this PR changes the return value of the data pipeline requests. Would you consider that a breaking change constituting a major version bump ? Do we consider requests to be part of the public API ? |
| @connection.request(params) | ||
| response = @connection.request(params) | ||
|
|
||
| unless response.body.empty? |
There was a problem hiding this comment.
@lanej Won't this part mean that we still end up with decoded bodies? Or am I misunderstanding your question?
| @@ -24,14 +24,35 @@ def create_pipeline(unique_id, name, description=nil, tags=nil) | |||
| :body => Fog::JSON.encode(params), | |||
| :headers => { 'X-Amz-Target' => 'DataPipeline.CreatePipeline' }, | |||
| }) | |||
|
|
|||
| Fog::JSON.decode(response.body) | |||
There was a problem hiding this comment.
@geemus this is what I was referring to. These requests used to return decoded response bodies. Now they return the entire response object.
There was a problem hiding this comment.
Ah. I'm open to either way. Not major-bumping is obviously easier. I think this stuff is relatively new, so that makes it feel a bit more flexible to change. But I agree it could break things. What is your inclination?
There was a problem hiding this comment.
My inclination is release a major bump and be good semver people.
There was a problem hiding this comment.
Works for me, I think. My only concern would be about how weird it might be (or not) for fog-core to be in a different major release level. They are distinct projects now, so probably fine, but just something to note.
No description provided.