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

add support for cloud-nuke-excluded tag in Cloudtrail resource #839

Closed
wants to merge 1 commit into from

Conversation

ErezWeiss
Copy link

add support for cloud-nuke-excluded tag in Cloudtrail resource

missing test, need some help here

Copy link
Contributor

@james03160927 james03160927 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will trigger tests.

@james03160927
Copy link
Contributor

Hi @ErezWeiss, did you test your change? I'm seeing this error during build time.

aws/resources/cloudtrail.go:26:11: undefined: util
aws/resources/cloudtrail.go:26:48: trailInfo.Tags undefined (type "github.com/aws/aws-sdk-go-v2/service/cloudtrail/types".TrailInfo has no field or method Tags)

Copy link
Contributor

@james03160927 james03160927 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the build failure + test appropriate things.

@@ -23,6 +23,7 @@ func (ct *CloudtrailTrail) getAll(c context.Context, configObj config.Config) ([
for _, trailInfo := range page.Trails {
if configObj.CloudtrailTrail.ShouldInclude(config.ResourceValue{
Name: trailInfo.Name,
Tags: util.ConvertTypesTagsToMap(trailInfo.Tags),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resource you are iterating over lacks the 'Tags' field. You need to call this method for each resource first.

@james03160927
Copy link
Contributor

Friendly ping @ErezWeiss .

@ErezWeiss
Copy link
Author

Friendly ping @ErezWeiss .

@james03160927 thanks for that, hopefully I'll handle it in the upcoming weeks

@james03160927
Copy link
Contributor

Got it. SGTM. Thanks

@james03160927
Copy link
Contributor

Friendly ping @ErezWeiss

@james03160927
Copy link
Contributor

HI @ErezWeiss, friendly ping. Do you plan to continue working on this change?

@james03160927
Copy link
Contributor

I'll take over the change and merge it in! 👍 since we haven't heard back from you for a while.

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.

3 participants