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: add CloudWatchLogGroup RetentionInDays #489

Closed
wants to merge 6 commits into from

Conversation

ayogun
Copy link
Contributor

@ayogun ayogun commented Jan 2, 2025

I intended to add RetentionInDays property to CloudWatchLogGroup

Copy link
Owner

@ekristen ekristen left a comment

Choose a reason for hiding this comment

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

@ayogun I'm not quite sure I understand what you are going for here? It's not clear to me why you are modifying the response from AWS? If RetentionInDays is nil coming from AWS why are you wanting to set it to 0? Is it just so that the value is present and you can make the property appear for filtering?

resources/cloudwatchlogs-loggroups.go Outdated Show resolved Hide resolved
@ayogun
Copy link
Contributor Author

ayogun commented Jan 2, 2025

@ayogun I'm not quite sure I understand what you are going for here? It's not clear to me why you are modifying the response from AWS? If RetentionInDays is nil coming from AWS why are you wanting to set it to 0? Is it just so that the value is present and you can make the property appear for filtering?

Hey Erik, it's my first time I touch GO and I'm trying my way out of here. This is why it was a draft PR. I'm also not sure what I am doing but trying to achieve that outcome:

  • I will be able to filter out my CW Log groups according to their retention period.

What do you think about PR now? Should I delete set to 0 part?

Anyway, thank you very much for due diligence!!!

@ekristen
Copy link
Owner

ekristen commented Jan 2, 2025

@ayogun welcome welcome welcome! Thanks for letting me know. I have a couple more suggestions for you, so we can do it a couple different ways. I can make a few more suggestions on the PR, or I can do the modifications on your PR for you to see.

@ayogun
Copy link
Contributor Author

ayogun commented Jan 2, 2025

@ayogun welcome welcome welcome! Thanks for letting me know. I have a couple more suggestions for you, so we can do it a couple different ways. I can make a few more suggestions on the PR, or I can do the modifications on your PR for you to see.

OMG, you are awesome! Please do the changes, and let me learn by watching the master ❤️

BTW, Happy happy new year!

@ekristen
Copy link
Owner

ekristen commented Jan 2, 2025

lol, you are too kind. I'll do the changes later, probably won't be for a couple of hours. Happy New Year!

@ekristen
Copy link
Owner

ekristen commented Jan 3, 2025

@ayogun sorry I didn't get to this yesterday something came up, I will today though.

@ayogun
Copy link
Contributor Author

ayogun commented Jan 3, 2025

@ayogun sorry I didn't get to this yesterday something came up, I will today though.

No worries. I can't wait to work with you! 🤩

@ayogun
Copy link
Contributor Author

ayogun commented Jan 3, 2025

Alt Text

@ekristen
Copy link
Owner

ekristen commented Jan 3, 2025

Closing this one as I can't manipulate your main branch, see the #494, I added a few more things while I was there.

@ekristen ekristen closed this Jan 3, 2025
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