Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

(features)The delete function for cloudwatch rum is added #952

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

wei-zhong90
Copy link
Contributor

Just add the delete function for cloudwatch-rum appmonitor resource

@wei-zhong90 wei-zhong90 requested a review from a team as a code owner March 3, 2023 15:34
@dza89
Copy link

dza89 commented Mar 6, 2023

example output dryrun:

eu-central-1 - CloudWatchRumApp - dummy - would remove

example output nodryrun:

eu-central-1 - CloudWatchRumApp - dummy - triggered remove
eu-central-1 - CloudWatchRumApp - dummy - waiting
eu-central-1 - CloudWatchRumApp - dummy - removed

@dza89
Copy link

dza89 commented Apr 7, 2023

@der-eismann

Can this be merged? Pretty straightforward.

@sstoops
Copy link
Contributor

sstoops commented May 1, 2023

Hi @dza89 ,

I'm not a maintainer, but I will make the observation that you've implemented the String() method when that has in fact been deprecated in favor of the Properties() method. The latter gives you the chance to return much more metadata about the resource allowing the end user to filter on any of these attributes. See the contributing guidelines for more details: https://github.com/rebuy-de/aws-nuke/blob/main/CONTRIBUTING.md#use-properties-instead-of-string-functions

Copy link
Member

@der-eismann der-eismann left a comment

Choose a reason for hiding this comment

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

Hey @wei-zhong90, thanks for your PR and sorry for the late reply! Could you implement the small change I suggested so we can get this merged?

resources/cloudwatch-rum.go Outdated Show resolved Hide resolved
wei-zhong90 and others added 3 commits August 28, 2023 09:13
Change the naming convention to conform to the general style as suggested

Co-authored-by: Philipp Trulson <[email protected]>
@der-eismann der-eismann merged commit 0dd135f into rebuy-de:main Aug 29, 2023
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants