-
Notifications
You must be signed in to change notification settings - Fork 22
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
Feature/regex matching #105
base: main
Are you sure you want to change the base?
Conversation
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
TODOs
|
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
we need to wait for rds_instances to actually be deleted before continuing with rds subnet deletion, because you cannot delete a subnet that contains an instance Co-authored-by: Nick Rohn <nrohn@vmware.com> Co-authored-by: Jhonathan Aristizabal <jhonathana@vmware.com>
We believe that security groups need to be deleted after the db instance is. And that vpc should be deleted last Co-authored-by: Nick Rohn <nrohn@vmware.com> Co-authored-by: Jhonathan Aristizabal <jhonathana@vmware.com>
feature: wait for rds_instances to be deleted
Update help section with new command
- remove vendor Co-authored-by: Jhonathan Aristizabal <jhonathana@vmware.com> Co-authored-by: Nick Rohn <nrohn@vmware.com>
- revert openstack dependency Authored-by: Nick Rohn <nrohn@vmware.com>
…rs into feature/regex-matching
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Authored-by: Nick Rohn <nrohn@vmware.com>
Hi @genevieve, if we wanted to get this merged upstream I assume we'll need to reintroduce vendoring and add more test coverage? Would you also want us to refactor the regex filter flag? We can also maintain our own fork if that's preferable. Thank you!! |
Hey Nick! Yes, if you wouldnt mind reintroducing vendoring and adding more test coverage, we can merge these changes! |
Thanks! I'll try to get it ready to be merged in a couple weeks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far!
Has this been tested with invalid regex input yet? I didn't see any code catching the panic from MustCompile
, but there were also 2205 file changes so I could have missed something.
@@ -27,7 +27,7 @@ func NewFolders(client client, logger logger) Folders { | |||
|
|||
// List not only lists top-level folders, it also lists child and grandchild | |||
// folders, and all VMs within those folders. | |||
func (v Folders) List(filter string, rType string) ([]common.Deletable, error) { | |||
func (v Folders) List(filter string, rType string, regex bool) ([]common.Deletable, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I am no longer aware of any users who are using leftovers
to delete folders on a multitenant vSphere cluster (since the Toolsmiths team no longer provides multitenant vSpheres to MAPBU). So I think you can safely implement deleting vSphere folders by regex, if that's something you want to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I'll need to find a good way to deal with the potential panic in common/resource_filter.go
before we merge.
@@ -32,6 +33,7 @@ func GetLeftovers(logger *app.Logger, o app.Options) (leftovers, error) { | |||
case app.Azure: | |||
l, err = azure.NewLeftovers(logger, o.AzureClientID, o.AzureClientSecret, o.AzureSubscriptionID, o.AzureTenantID) | |||
case app.GCP: | |||
fmt.Println("in gcp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to self and Gen: make sure this test line is gone before merging the PR
Thank you for the review @rowanjacobs. I still need to get around to getting this ready for merge. I'm trying to find time |
cc050b9
to
36e523c
Compare
We would like to be able to match using regex. I'm opening this as a draft since we plan to continue working / redesigning before trying to get our changes merged in :).
Example use case:
This PR also reorders some AWS resource deletion by their type, and adds waits to a couple resources so we don't fail improperly (especially important for RDS, which takes ~10 minutes to delete)
Thanks!
cc @ciriarte