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

Permit use of service role with change set workflow #70

Merged
merged 6 commits into from
Nov 19, 2020
Merged

Permit use of service role with change set workflow #70

merged 6 commits into from
Nov 19, 2020

Conversation

nullus
Copy link
Contributor

@nullus nullus commented Jul 30, 2019

Add --service-role-arn option to "change-set create"

Add --service-role-arn option to "change-set create"
Copy link

@trembylene trembylene left a comment

Choose a reason for hiding this comment

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

Hi Nullus!

This functionality is a good addition, thank you for your PR.

Before I can merge this however, the PR needs two more things.
Can you please update the documentation to reflect the addition of the new functionality, and write a test to cover this use case?

Thank you,
Jess

@nullus
Copy link
Contributor Author

nullus commented Aug 27, 2019

Can you please update the documentation to reflect the addition of the new functionality

Absolutely, do you want that in the README.md? Any preference on whether it's mentioned under the existing usage of --service-role-arn or added under "Change-set support"?

... write a test to cover this use case?

I couldn't see any equivalent tests for that parameter for the existing use case, so I didn't add one for this. Where should that be added?

@mukaibot
Copy link
Contributor

mukaibot commented Aug 29, 2019

Can you please update the documentation to reflect the addition of the new functionality

Absolutely, do you want that in the README.md? Any preference on whether it's mentioned under the existing usage of --service-role-arn or added under "Change-set support"?

Change-set support sounds good! I think most people will look for it there. Thanks!

... write a test to cover this use case?

I couldn't see any equivalent tests for that parameter for the existing use case, so I didn't add one for this. Where should that be added?

Yeah, we don't have tests for change-sets! @trembylene and I are new to the project so we're still catching up on some things. Would you feel comfortable creating a change_set_spec.rb file that tests only your needed parameter? Something simple is fine, we don't expect you to write tests for the whole change set feature. I guess that's on us!

@lukeck
Copy link
Contributor

lukeck commented Nov 19, 2020

Thanks for adding tests @amcinnes! Looks good.

@lukeck lukeck merged commit 33e910f into realestate-com-au:master Nov 19, 2020
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.

None yet

6 participants