Skip to content

Conversation

@maintux
Copy link
Contributor

@maintux maintux commented Feb 21, 2022

Which issue(s) does this change fix?

#2249

Why is this change necessary?

The PR #3249 was not complete

How does it address the issue?

And a custom exporter for StackSet resource

What side effects does this change have?

I think there are no side effects because is a new feature

Checklist

  • Add input/output type hints to new functions/methods
  • Write design document (Do I need to write a design document?)
  • Write unit tests
  • Write/update functional tests
  • Write/update integration tests
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@maintux
Copy link
Contributor Author

maintux commented Feb 21, 2022

Hi guys, I can't figure out why the win build fails with a permission denied error on tests. This error doesn't happen on *nix os. Could you help me to find out the problem?

Thanks

@maintux
Copy link
Contributor Author

maintux commented Feb 22, 2022

Now the test is fixed. The continuous-integration/appveyor/pr/make-pr check is blocked on appveyor but it should be ok.
The PR is ready for review.

@maintux
Copy link
Contributor Author

maintux commented Mar 15, 2022

@mildaniel @moelasmar @jfuss any news on this? It's a bug fix and is quite urgent for us.
Thanks.

Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

Hi @maintux Thanks for your contribution. Overall LGTM. But would you please also add some integration tests? I believe that would be similar to the nested stack ones? e.g. test_package_with_deep_nested_template under tests/integration/package/test_package_command_zip.py and test_deploy_nested_stacks under tests/integration/deploy/test_deploy_command.py. Thanks!

@maintux
Copy link
Contributor Author

maintux commented Mar 30, 2022

Hi @hawflau, thanks for your reply. As requested I've added the missing integration tests.

Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

template_path = resource_dict.get(self.PROPERTY_NAME, None)

if template_path is None or is_s3_url(template_path):
# Nothing to do
Copy link
Contributor

Choose a reason for hiding this comment

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

A debug logging could be nice here to catch this case.

@hawflau hawflau merged commit 1a2c0e3 into aws:develop Mar 30, 2022
mndeveci pushed a commit to mndeveci/aws-sam-cli that referenced this pull request Apr 5, 2022
* add exporter for AWS::CloudFormation::StackSet resource

* fix access denied error on windows

* fix line too long

* fix file formatting

* Add integration tests for StackSet exporter

* black reformat

Co-authored-by: Massimo Maino <massimo.maino@claudacademy.com>
Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants