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

Added properties for several AWS resources #980

Closed
wants to merge 2 commits into from
Closed

Conversation

nwmqpa
Copy link

@nwmqpa nwmqpa commented Apr 19, 2023

No description provided.

@nwmqpa nwmqpa requested a review from a team as a code owner April 19, 2023 10:06
@der-eismann der-eismann changed the title ✨ Added properties for several AWS resources Added properties for several AWS resources Apr 19, 2023
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 @nwmqpa! Sorry for the late reply. Are you still interested in getting your changes in? It would be great if you could implement my suggestions then.

Comment on lines +41 to +44
if err != nil {
return nil, err
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
return nil, err
}

I think this was a copy-paste error

Comment on lines +49 to +53
func (e *IAMSAMLProvider) Properties() types.Properties {
properties := types.NewProperties()
return properties
}

Copy link
Member

Choose a reason for hiding this comment

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

This should have one property at least

func (s *SFNStateMachine) Properties() types.Properties {
properties := types.NewProperties()

properties.Set("Name", s.name)
Copy link
Member

Choose a reason for hiding this comment

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

This should have the ARN as well

@der-eismann der-eismann added the status/waiting-reponse Waiting for the issue author to respond to a question. label Aug 25, 2023
@nwmqpa
Copy link
Author

nwmqpa commented Aug 29, 2023

Hey @der-eismann, I'll try to implement those soon

@ekristen
Copy link
Collaborator

@nwmqpa due to the size and the number of resources touched here and that the fork as since diverged a bit, I cannot pull these over cleanly. If you'd like to open a PR on https://github.com/ekristen/aws-nuke it would be welcome, please read the contributing guide over there.

Otherwise, It's on my backlog to add properties to all resources. Thanks.


Please see the copy of the notice from the README about the deprecation of this project. Sven was kind enough to grant me access to help triage and close issues and pull requests that have already been addressed in the actively maintained fork. Some additional information is located in the welcome issue for more information.

Caution

This repository for aws-nuke is no longer being actively maintained. We recommend users to switch to the actively maintained fork of this project at ekristen/aws-nuke.
We appreciate all the support and contributions we've received throughout the life of this project. We believe that the fork will continue to provide the functionality and support that you have come to expect from aws-nuke.
Please note that this deprecation means we will not be addressing issues, accepting pull requests, or making future releases from this repository.
Thank you for your understanding and support.

@ekristen ekristen closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/waiting-reponse Waiting for the issue author to respond to a question.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants