Cleantool #234
Cleantool #234
Conversation
Move the flags out of the test file to the e2e package, and export flag variables and parseFlagsOrDie.
Relocate and export FallbackCleanup function.
briantkennedy
force-pushed
the
cleantool
branch
from
90ad9d2 to
decefc6
Compare
|
friendly ping @bowei @kl52752 @AwesomePatrol |
cmd/e2e-cleaner/main.go
Outdated
| } | ||
|
|
||
| func eMain() error { | ||
| //klog.InitFlags(nil) |
There was a problem hiding this comment.
nit: this comment is not needed
cmd/e2e-cleaner/main.go
Outdated
| } | ||
|
|
||
| func main() { | ||
| if err := eMain(); err != nil { |
There was a problem hiding this comment.
eMain doesn't seem to remove any errors. Please consider consolidating the two
There was a problem hiding this comment.
combined with eMain
e2e/testmain_test.go
Outdated
| "google.golang.org/api/googleapi" | ||
| "google.golang.org/api/impersonate" | ||
| "google.golang.org/api/option" | ||
| ) | ||
|
|
||
| var ( | ||
| // theCloud is a global to be used in the e2e tests. | ||
| theCloud cloud.Cloud |
There was a problem hiding this comment.
nit: it seems it is unused
There was a problem hiding this comment.
I'm unsure what action this is suggesting
There was a problem hiding this comment.
as you reverted from TheCloud to theCloud now there is a duplicate declaration of global variable with this name. Declaration here can be removed
There was a problem hiding this comment.
Thanks for calling this out, it's been a while, and I forgot that go build doesn't build tests for some reason =/
e2e/cleanup.go
Outdated
| log.Printf("FallbackCleanup: TheCloud.TcpRoutes().List(ctx, _): %v\n", err) | ||
| return | ||
| } | ||
| log.Printf("Listed %d tcproutes", len(tcprs)) |
There was a problem hiding this comment.
nit: this looks like a debug log I think we can get rid of it
e2e/addresses_test.go
Outdated
| @@ -42,16 +42,16 @@ func TestAddresses(t *testing.T) { | |||
|
|
|||
| t.Logf("addr1 = %s", fmt.Sprint(addr1)) | |||
|
|
|||
| t.Cleanup(func() { theCloud.Addresses().Delete(context.Background(), addr1Key) }) | |||
| t.Cleanup(func() { TheCloud.Addresses().Delete(context.Background(), addr1Key) }) | |||
There was a problem hiding this comment.
Please consider not changing the variable name. It doesn't seem needed (I run sed and the code still worked). It would make PR diff much smaller
There was a problem hiding this comment.
reverted to theCloud
briantkennedy
force-pushed
the
cleantool
branch
from
decefc6 to
ef9bbae
Compare
|
/lgtm |
* Fix a bug in resource cleanup where tcproutes and meshes were not being cleaned up due to sending invalid URL. * Adjust cleanup matching to clean resources from all tests when RunID is empty.
briantkennedy
force-pushed
the
cleantool
branch
from
ef9bbae to
8aadc2d
Compare
|
/lgtm |
|
/approve |
google-oss-prow
bot
merged commit da81e2a
into
GoogleCloudPlatform:master
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AwesomePatrol, bowei, briantkennedy, kl52752 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Adds a tool to help manually clean up orphaned test resources.
Fixes #232