-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Deprecation resolution for ResourceTypes #61
Comments
I may be holding it incorrectly, but another thing I noticed was that I didn't see the deprecated resource type warning when using the CLI. I saw this when I created a libnuke config and passed a logger. In aws-nuke https://github.com/ekristen/aws-nuke/blob/v3.1.0/pkg/commands/nuke/nuke.go#L73-L76, no logger is passed and libnuke seems to create a discard logger for that in https://github.com/ekristen/libnuke/blob/v0.16.0/pkg/config/config.go#L97-L101, leading to no visible deprecation log even if I enable debug log with |
This was the original behavior, debatable if that should be changed. It's one thing to morph a bad config another to include a resource that might not meant to have been included. What do you think about throwing an error if using the a deprecated alias OR a non-existent resource when using includes/excludes?
It does look like I have a bug in aws-nuke proper where I'm not passing in the logger, so I do need to fix that. |
For non-existing resource types, throwing error seems better as the provided resource is not known and has no use. But I think that's different from deprecated alias. Deprecated aliases are well defined, finite and strictly map to their new values. There's less of a change of anything going wrong unless multiple things have the same alias. Compared to the current behavior, automatic migration or returning error will make it much easier to figure out what went wrong in the scenario I had. |
While migrating from rebuy-de/aws-nuke to libnuke in fluxcd/test-infra#39, aws-nuke failed to delete EKSCluster. There were multiple errors. Some of them related to removing role from instance profile that lead me to rebuy-de/aws-nuke#702, which didn't lead to any conclusion. But @ekristen your last comment about limiting the resource types lead me to checking the includes that I have configured.
Coming from rebuy-de/aws-nuke, I had
EKSNodegroups
in my configuration. I did see the warningbut the listed resources didn't contain any nodegroup. When I used the new name, it worked perfectly.
In
Config.ResolveDeprecations()
, refer https://github.com/ekristen/libnuke/blob/v0.16.0/pkg/config/config.go#L239-L255, where this conversion takes place, only theAccount.Filters
are updated with the new replacement. TheAccount.ResourceTypes.Includes
and other fields remain as they are. I'm not sure if this is intended but I tried replacing the includes as well in a local copy and that made it work as expected. Is this the intended behavior for deprecation resolution?The text was updated successfully, but these errors were encountered: