Skip to content
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

feat(pkg/config/filter): only warn on date parse error #1245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mbarneyjr
Copy link

In current state when you use a dateOlderThan filter, it'll return an error if it comes across a resource/field that it cannot parse as a date, and will prevent aws-nuke from running on all of the other resources it may have successfully processed

This change will instead only log a warning when an error occurs, which the user will be able to see. The resource will not be filtered out (although I'm not opinionated either way, my main goal is to not hold up aws-nuke from running because one resource was misconfigured)

I'm open to feedback/change requests

Here's an example output with these changes:

Do you really want to nuke the account with the ID 123456789012 and the alias 'my-account'?
Waiting 3s before continuing.
time="2024-07-11T21:22:47Z" level=warning msg="Failed to parse date IAMNOTADATE: unable to parse time IAMNOTADATE"
us-west-1 - DynamoDBTable - testing - [Identifier: "testing", tag:expiration-date: "IAMNOTADATE"] - would remove
Scan complete: 1 total, 1 nukeable, 0 filtered.

The above resources would be deleted with the supplied configuration. Provide --no-dry-run to actually destroy resources.

@mbarneyjr mbarneyjr requested a review from a team as a code owner July 12, 2024 13:39
@ekristen
Copy link
Contributor

Interesting idea. I generally like it, however this might be considered a breaking change as it would change the default behavior of the tool. I'm trying to think of any ramifications.

@mbarneyjr
Copy link
Author

I wouldn't feel opposed to a could be a config option to enable this functionality, leaving the previous functionality as the default, if that's the approach you'd like to go with I'd be happy to push another change to this PR

@mbarneyjr
Copy link
Author

@ekristen I've pushed a change to add a nuke-on-date-parse-error feature flag, that if left default (false), will cause aws-nuke to behave exactly the same as it did before this change. I'm happy to tweak anything you'd like to get this merged

@mbarneyjr
Copy link
Author

@ekristen just checking if there's anything we can do to get this merged, I'm happy to make any changes if there's anything you have in mind

@ekristen
Copy link
Contributor

@mbarneyjr I have no control over this unfortunately. I'm only a contributor. I only try and help folks here as I can. I've started my own managed fork over at https://github.com/ekristen/aws-nuke that's diverged greatly from this in terms of test-ability and core code.

@mbarneyjr
Copy link
Author

Interesting, would you say this repo is unmaintained? Looking at the commit history it's mostly bot activity, April ec40ba being the last regular push

@ekristen
Copy link
Contributor

It's less maintained then mine, hence why I split off. They do bulk maintenance every few months, so it's not completely unmaintained. Just my two cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants