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

Move account process to main SAR template #487

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

sbkok
Copy link
Collaborator

@sbkok sbkok commented Aug 17, 2022

Why?

The SAR does not understand local references as introduced by the src/account_processing.yml unfortunately.

What?

After investigating several options, the only option that was accepted was to move the logic inside the main template instead. The references are updated to use the ones available in the main template directly.

Additionally, the ARN references are updated to include the partition correctly.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sbkok sbkok added this to the v3.2.0 milestone Aug 17, 2022
@StewartW
Copy link
Contributor

Makes sense.
Shame that sam publish and sam deploy have different reactions to this :(

StewartW
StewartW previously approved these changes Aug 17, 2022
**Why?**

The SAR does not understand local references as introduced by the
src/account_processing.yml unfortunately.

**What?**

After investigating several options, the only option that was accepted was to
move the logic inside the main template instead.
The references are updated to use the ones available in the main template
directly.

Additionally, the ARN references are updated to include the partition correctly.
Removed from the CloudFormation Linter configuration and removed the file
itself. I've added this as a separate commit to make it easier to check the
differences between the `src/account_processing.yml` and the resources that it
contained that were moved to the `src/template.yml` file.
@sbkok
Copy link
Collaborator Author

sbkok commented Aug 19, 2022

(Rebased on the latest commit in master to resolve a conflict)

StewartW
StewartW previously approved these changes Aug 19, 2022
javydekoning
javydekoning previously approved these changes Aug 22, 2022
Copy link
Contributor

@javydekoning javydekoning left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@sbkok sbkok dismissed stale reviews from javydekoning and StewartW via 6736fae August 23, 2022 15:11
@sbkok
Copy link
Collaborator Author

sbkok commented Aug 23, 2022

@javydekoning and @StewartW, I had to resolve a merge conflict. Unfortunately that dismissed the review approvals.

@sbkok sbkok merged commit 095c086 into awslabs:master Aug 24, 2022
@sbkok sbkok deleted the fix/account-mgmt-state-machine branch August 24, 2022 14:05
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