Issue 6948051: Add simple tool for deleting security groups
DescriptionAdd simple tool for deleting security groups
Basic test of using the current api for doing some actual task. As the
live tests create lots of security groups without ever deleting them
it's also sort of useful to have around.
https://code.launchpad.net/~gz/goose/secgroup-delete-all/+merge/139991
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : Add simple tool for deleting security groups #
Total comments: 16
Patch Set 3 : Add simple tool for deleting security groups #Patch Set 4 : Add simple tool for deleting security groups #
MessagesTotal messages: 12
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM but I'd like you to consider the requested change. ie construct the Openstack client outside of the DeleteAll function. The client is setup to point to a live instance or service double as required. https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/ser... testservices/novaservice/service_http.go:760: writeResponse(w, http.StatusAccepted, nil) The branch I just landed in goose fixes this already https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:865: c.Assert(resp.StatusCode, Equals, http.StatusAccepted) This is fixed by a branch I just landed https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... File tools/secgroup-delete-all/main.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:15: creds, err := identity.CompleteCredentialsFromEnv() I would consider passing the credentials as a parameter. This would avoid the need to hack the fake credentials into env for the service double tests. This pattern is used already for the various test setups for nova, swift etc testing. Actually, I would even consider passing in the client and have the caller (either the main() or test setup) construct the desired client, pointing to whatever backend is required, either live instance or service double https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... File tools/secgroup-delete-all/main_test.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main_test.go:37: os.Setenv("OS_REGION_NAME", region) The above won't be needed if you create the credentials and client during test setup and then pass the client to the DeleteAll function. See comment in that function for more. https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main_test.go:44: computeurl := s.Server.URL + "/v2.0/1" It's not so much a magic url as such, rather teaching the identity service the correct url for a nova service endpoint. There should be a function on the nova service double to do this, rather than exposing it to the test author.
Sign in to reply to this message.
Overall LGTM. I think we have some feedback, but it has already taken a long time to get this, and people can't use it if it isn't available. I think doing stuff like this really helps us exercise our library in a case where it isn't just a bunch of test cases. So spending a little bit of time to make it clear what is good practice for using the library is worthwhile, but I'd also like to just have a hacky "DeleteAll" tool so that we can get other work done. So, IMO, feel free to find your own balance point of "polish our test double infrastructure" vs "get a tool out that we can use". You've certainly exposed things that we want to do (not hard-coding URLs, easier injection of state and errors, etc.) We don't have to do all of those things before this lands. https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... File testservices/novaservice/actions.go (right): https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... testservices/novaservice/actions.go:7: // GZ 2013-01-21: This should take map[int]interface{} but go disagrees You can't simply cast a map[int]Foo to a map[int]interface{} because the types have different sizes. So the internal map content would be at different locations. You can iterate one map and shove all the content into the other, or you can point to the overall map with an interface{}, but there is a specific reason for it. If you really wanted to make this generic, you could probably get away with something from reflect: http://golang.org/pkg/reflect/#Value.MapKeys https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... File tools/secgroup-delete-all/main.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:15: creds, err := identity.CompleteCredentialsFromEnv() Yeah, it has been factored to the point of having an io.Writer rather than writing to stdout directly, we could certainly see a simple "I have an authed client, call the methods on it." Essentially just move the code from creds, err := to the end of osc := client.NewClient into a separate helper. (Possibly even at the level of nova.New?) https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... File tools/secgroup-delete-all/main_test.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main_test.go:44: computeurl := s.Server.URL + "/v2.0/1" I think Ian is correct that we shouldn't have to do this here, but I'm fine doing it until we fix the nova double.
Sign in to reply to this message.
https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... File tools/secgroup-delete-all/main_test.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main_test.go:44: computeurl := s.Server.URL + "/v2.0/1" On 2013/01/22 06:23:59, jameinel wrote: > I think Ian is correct that we shouldn't have to do this here, but I'm fine > doing it until we fix the nova double. This is fixed in the branch I have in progress.
Sign in to reply to this message.
BTW this should make a go installable tool just fine. go install launchpad.net/goose/tools/secgroup-delete-all https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... File testservices/novaservice/actions.go (right): https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... testservices/novaservice/actions.go:8: func generateId(existing_objects map[int]nova.SecurityGroup) int { rather than this, i'd be tempted to have a groupid counter and simply increment it every time, so group ids are never reused. that has the advantage that the code is simpler (and generating a new group id is O(1) not O(len(n.groups)), not that that will make any significant difference, of course). it can also be an advantage when debugging. https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... File tools/secgroup-delete-all/main.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:15: creds, err := identity.CompleteCredentialsFromEnv() On 2013/01/22 06:23:59, jameinel wrote: > Yeah, it has been factored to the point of having an io.Writer rather than > writing to stdout directly, we could certainly see a simple "I have an authed > client, call the methods on it." > > Essentially just move the code from creds, err := to the end of osc := > client.NewClient into a separate helper. (Possibly even at the level of > nova.New?) +1 https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:31: return err i doubt you want this still here. i'd log the message rather than returning it, so even if you fail to delete one group, the others are deleted. that means it might be worth returning a bool rather than an error, because returning []error would seem like overkill. https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:32: failed += 1 failed++ https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:34: deleted += 1 deleted++ https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... tools/secgroup-delete-all/main.go:44: fmt.Fprintf(w, "%d security groups could not be deleted.\n", failed) if you're logging error messages, there's probably no need for this message. https://codereview.appspot.com/6948051/diff/3001/tools/tools.go File tools/tools.go (right): https://codereview.appspot.com/6948051/diff/3001/tools/tools.go#newcode1 tools/tools.go:1: package tools what's this file/package for?
Sign in to reply to this message.
oh, LGTM BTW, with the considerations below. On 2013/01/22 07:46:09, rog wrote: > BTW this should make a go installable tool just fine. > > go install launchpad.net/goose/tools/secgroup-delete-all > > https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... > File testservices/novaservice/actions.go (right): > > https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... > testservices/novaservice/actions.go:8: func generateId(existing_objects > map[int]nova.SecurityGroup) int { > rather than this, i'd be tempted to have a groupid counter and simply increment > it every time, so group ids are never reused. that has the advantage that the > code is simpler (and generating a new group id is O(1) not O(len(n.groups)), not > that that will make any significant difference, of course). > > it can also be an advantage when debugging. > > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > File tools/secgroup-delete-all/main.go (right): > > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:15: creds, err := > identity.CompleteCredentialsFromEnv() > On 2013/01/22 06:23:59, jameinel wrote: > > Yeah, it has been factored to the point of having an io.Writer rather than > > writing to stdout directly, we could certainly see a simple "I have an authed > > client, call the methods on it." > > > > Essentially just move the code from creds, err := to the end of osc := > > client.NewClient into a separate helper. (Possibly even at the level of > > nova.New?) > > +1 > > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:31: return err > i doubt you want this still here. > i'd log the message rather than returning it, so even if you fail to delete one > group, the others are deleted. > that means it might be worth returning a bool rather than an error, because > returning []error would seem like overkill. > > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:32: failed += 1 > failed++ > > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:34: deleted += 1 > deleted++ > > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:44: fmt.Fprintf(w, "%d security groups could > not be deleted.\n", failed) > if you're logging error messages, there's probably no need for this message. > > https://codereview.appspot.com/6948051/diff/3001/tools/tools.go > File tools/tools.go (right): > > https://codereview.appspot.com/6948051/diff/3001/tools/tools.go#newcode1 > tools/tools.go:1: package tools > what's this file/package for?
Sign in to reply to this message.
On 2013/01/22 02:03:37, wallyworld wrote: > > https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/ser... > testservices/novaservice/service_http.go:760: writeResponse(w, > http.StatusAccepted, nil) > The branch I just landed in goose fixes this already We'd discussed the return codes in the standup, but I should have twigged and merged your branch in perhaps. :) > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:15: creds, err := > identity.CompleteCredentialsFromEnv() > I would consider passing the credentials as a parameter. This would avoid the > need to hack the fake credentials into env for the service double tests. This > pattern is used already for the various test setups for nova, swift etc testing. > Actually, I would even consider passing in the client and have the caller > (either the main() or test setup) construct the desired client, pointing to > whatever backend is required, either live instance or service double This was the obvious change to make (along with passing in io.Writer), I didn't do it straight off for two reasons: * Moving into main currently means more untested logic * Wanted to see what the testing would look like with all client parts included I'm not sure this is the sort of thing we want to ever have a live test for, it's a tool for nuking stuff. That does impact the way you think about writing the tests though, I really didn't want to do all the setup in a as-if-running-against-live-instance fashion which perhaps doesn't really make sense for our common case.
Sign in to reply to this message.
On 2013/01/22 07:46:09, rog wrote: > BTW this should make a go installable tool just fine. > > go install launchpad.net/goose/tools/secgroup-delete-all Yeah, had worked this out but didn't update the mp description. Needing to remember to ./... things keeps on getting me. > https://codereview.appspot.com/6948051/diff/3001/testservices/novaservice/act... > testservices/novaservice/actions.go:8: func generateId(existing_objects > map[int]nova.SecurityGroup) int { > rather than this, i'd be tempted to have a groupid counter and simply increment > it every time, so group ids are never reused. that has the advantage that the > code is simpler (and generating a new group id is O(1) not O(len(n.groups)), not > that that will make any significant difference, of course). > > it can also be an advantage when debugging. That's pretty much what the code within the service does, and probably even though doing everything through the API makes for crappy tests in this case, just using that may still be less painful. > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:15: creds, err := > identity.CompleteCredentialsFromEnv() > On 2013/01/22 06:23:59, jameinel wrote: > > Yeah, it has been factored to the point of having an io.Writer rather than > > writing to stdout directly, we could certainly see a simple "I have an authed > > client, call the methods on it." > > > > Essentially just move the code from creds, err := to the end of osc := > > client.NewClient into a separate helper. (Possibly even at the level of > > nova.New?) > > +1 Yeah, giving in on having this test much more than the trivial is I guess the easiest option, most of the code should be living, and tested, elsewhere in the end. > https://codereview.appspot.com/6948051/diff/3001/tools/secgroup-delete-all/ma... > tools/secgroup-delete-all/main.go:31: return err > i doubt you want this still here. > i'd log the message rather than returning it, so even if you fail to delete one > group, the others are deleted. > that means it might be worth returning a bool rather than an error, because > returning []error would seem like overkill. Good spot, this was a temporary debugging line that got committed by accident, written because I couldn't make a test to exercise the error condition. I was avoiding printing something for every failed group, because generally with juju if you've got one group you can't delete, you've got 7 others with the exact same issue, which is tedious. May be better even so. > https://codereview.appspot.com/6948051/diff/3001/tools/tools.go#newcode1 > tools/tools.go:1: package tools > what's this file/package for? `touch __init__.py`! I take it it's not required?
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM. I used to have a bash script for the same task - using the CLI nova client. I like this better, though :)
Sign in to reply to this message.
*** Submitted: Add simple tool for deleting security groups Basic test of using the current api for doing some actual task. As the live tests create lots of security groups without ever deleting them it's also sort of useful to have around. R=wallyworld, jameinel, rog, dimitern CC= https://codereview.appspot.com/6948051
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
