search

tumblr / collins

groovy kind of love
tumblr.github.com/collins
Apache License 2.0
571 stars 99 forks source link

Delete single ip address #505

Closed ssgelm closed 5 years ago

ssgelm commented 7 years ago

As discussed in #389, this adds support for deleting a single IP address via the api. Support is also added to collins-shell and collins-client for the newly added api call. I was looking to add the new address parameter to the API docs too but it looks like that only lives in the gh-pages branch so that will require a separate PR.

ssgelm commented 7 years ago

In lieu of the proper documentation (for which I can make a PR to gh-pages if this looks good to you all) this adds an address parameter to the DELETE /api/asset/:tag/addresses route. If address is passed, the address is deleted assuming it is currently allocated to the provided tag.

roymarantz commented 7 years ago

This looks real good. I'd like to get the documentation & logging setup before merging though. thanks

byxorna commented 7 years ago

This is awesome! Once @roymarantz's comments have been addressed, lgtm!

ssgelm commented 7 years ago

Docs change PRed in #508

ssgelm commented 7 years ago

@roymarantz and @byxorna I have pushed new commits to fix your concerns.

roymarantz commented 7 years ago

as I said in #508 should the code exclude specifying both a pool and address?

ssgelm commented 7 years ago

I'm not sure there's much of a risk to allow the end user to specify both. Address takes precedence over pool which makes sense to me as it's more specific. If a user were to specify both I can't imagine they would be surprised to see the single address deleted.

roymarantz commented 7 years ago

OK this is 👍 Thanks

ssgelm commented 7 years ago

Now that #508 is merged are you planning on merging this?

ssgelm commented 7 years ago

Cool, I'll make the changes you requested. Thanks for taking a look!

ssgelm commented 5 years ago

Sorry to bump this after so long but what concretely would you all like me to fix here? Just the ruby client? Adding unit tests seems like a nice to have but the rest of this code isn't tested yet so it seems like a good amount of work...

defect commented 5 years ago

Hey @ssgelm! Sorry for dropping the ball on this. I'm assigning myself to this PR to get this rolling again.

ssgelm commented 5 years ago

Cool thanks! More than happy to work on anything you'd like I just want to make sure we are on the same page. 😄

defect commented 5 years ago

We split out the ruby gems to their own repos (collins_client and collins_shell]) not too long ago, so i think a first step would be to move those changes over to those repos, or at least remove those changes from this PR since they're blocking merging.

Personally I don't think this PR should be blocked on adding it to the client libraries as long as we don't break current functionality, though it would be real nice to have support for it there :) But I think that can happen async from this.

ssgelm commented 5 years ago

@defect I removed the now defunct client changes and rebased off master. Do you think you could give this another look now?

defect commented 5 years ago

Again.. sorry for the delay :( This looks good to me. Let me see if i can get @byxorna 's eyes on it again since he had a few comments.

ssgelm commented 5 years ago

Looks like everyone is ok with this... can it be merged?

defect commented 5 years ago

Very sorry @ssgelm. 2+ years for a PR is quite embarrassing. I'll merge this tonight or tomorrow morning.