search

marpaia / chef-golang

Go library to interact with the Chef server API
Other
77 stars 31 forks source link

Added some logic to enable uploading cookbooks #34

Closed svanharmelen closed 10 years ago

svanharmelen commented 10 years ago

With this added code you can now post a cookbook to the community, or newly released supermarket, API. It does break backwards compatibility for post commands which with the current options seems the only way to make this work.

Please let's discus alternatives if this is not an acceptable approach. For example, one solution could be to make a PostCookbook method in addition to the Post method.

Cheers,

S.

marpaia commented 10 years ago

This unit test will break if I merge this: https://github.com/marpaia/chef-golang/blob/master/api_test.go#L225

@spheromak, we've gotten a bit of heat before for breaking the public API. what do you think?

svanharmelen commented 10 years ago

Hi @marpaia,

Updating the unit test is of course no problem (should have probably done that together with my initial PR), but at this point it's maybe better to first hear the opinion of @spheromak about this change...

So that if we need to find another way to get this functionality, I don't have to rewrite the tests twice :wink:

So would love to hear what you think about this PR @spheromak?

svanharmelen commented 10 years ago

Hi @marpaia and @spheromak, any update on your points of view over this one?

Thx!

spheromak commented 10 years ago

In General I don't like how the backend for requests are crafted in the current codebase. I was working on making things better, but it would require all the current api methods change. For me personally I am 100% ok with breaking how POST works :+1:

marpaia commented 10 years ago

@svanharmelen i'm cool with merging once the tests are modified

svanharmelen commented 10 years ago

@marpaia I just got around to updating the tests. I also added a line of code so that you can also send an empty string as the content type. If you do so, the header will not be set which will give the same behaviour as before this commit.

svanharmelen commented 10 years ago

Hi @marpaia and @spheromak, I would really love it if you can merge this PR now... I updated the tests as requested, so it seems there is no reason to not merge it now.

If anything changed and you do want me to update/change anything else, then please let me know so I can fix that and we can move ahead with this PR.

Thanks!

svanharmelen commented 10 years ago

As this now has a merge conflict, I will close this PR and open a new clean one instead.