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

Don't override default Rest Client delete method. #62

Merged
merged 1 commit into from
Feb 13, 2015

Conversation

ehelms
Copy link
Contributor

@ehelms ehelms commented Feb 12, 2015

This override breaks any other projects use of RestClient that relies
on this gem.

This override breaks any other projects use of RestClient that relies
on this gem.
@ehelms
Copy link
Contributor Author

ehelms commented Feb 12, 2015

This is a blocker for the Foreman/Katello project.

@daviddavis
Copy link

👍 for this fix

👎 for overriding delete in RestClient.

@pdilung
Copy link
Contributor

pdilung commented Feb 12, 2015

Hello All,

@ehelms 👍 Thanks for the fix.

I did this override and indeed your approach is way better than mine. Actually before I have overriden the delete I was thinking about implementing delete with payload but ... 😄

@abenari I think that it would also make sense to roll back to the older version of OVIRT::Client#http_delete and create one variant for delete with payload OVIRT::Client#http_delete_with_payload. Let me your opinion.

@ehelms
Copy link
Contributor Author

ehelms commented Feb 12, 2015

@pdilung It happens :) I thought of a few ways to try and tackle this and this seemed the least invasive while keeping your original intent in tact. Can you estimate a time frame for pushing out a gem with the fix? Mostly to know whether we should pin our Gemfile for a few days or not to right our builds.

@pdilung
Copy link
Contributor

pdilung commented Feb 12, 2015

@ehelms I would let @abenari and other stake holders to decide if a rollback to OVIRT::Client#http_delete should take place. I assume the updated gem will be pushed pretty fast. I have already implemented locally a rollback to previous version of OVIRT::Client#http_delete and a payload variant of DELETE method OVIRT::Client#http_delete_with_payload. I can create a pull request tomorrow before noon CET.

@pdilung
Copy link
Contributor

pdilung commented Feb 13, 2015

@ehelms Created a pull request #64 that contains a fix that you have introduced in #62 as well as my proposal.

abenari added a commit that referenced this pull request Feb 13, 2015
Don't override default Rest Client delete method.
@abenari abenari merged commit 8961745 into abenari:master Feb 13, 2015
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