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

[Resolves #1264] Update AWS CDK Example for CDK v2 and Asset Support #1266

Closed
wants to merge 9 commits into from

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Nov 24, 2022

Updates the AWS CDK example for CDK v2 and adds asset support.

PR Checklist

  • Wrote a good commit message & description [see guide below].
  • Commit message starts with [Resolve #issue-number].
  • Added/Updated unit tests.
  • Added/Updated integration tests (if applicable).
  • All unit tests (make test) are passing.
  • Used the same coding conventions as the rest of the project.
  • The new code passes pre-commit validations (pre-commit run --all-files).
  • The PR relates to only one subject with a clear title.
    and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

  • Before merge squash related commits.

Other Information

Guide to writing a good commit

@X-Guardian X-Guardian marked this pull request as ready for review November 24, 2022 10:31
Copy link
Contributor

@jfalkenstein jfalkenstein left a comment

Choose a reason for hiding this comment

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

So this documented example is starting to become less of a "recipe" like it was before and more of library code that needs to be copied and pasted. There's a lot going on here and, with the rise of CDK, I think it's important we make this easier to do. Here's what I think would be better:

What if you made a Template Handler that can be installed? We could potentially bootstrap a repo in the Sceptre Org for it. The template handler could work similar to the "file" handler that this example relies upon. Here's how it would be different, though:

  1. Rather than having a sceptre_handler() method returning a string, it would have a get_construct_class that returns the construct class.
  2. The handler itself would do the synth and asset publishing, returning the string
  3. If you wanted to be fancy, you could make it configurable so that it could EITHER take a python file as an argument OR it could use cdk synth to work with projects not in python, then get the published manifest and such from file. (My company's cdk people all insist on using CDK with typescript, despite nobody using typescript for other projects).

To give you an example of what making a template handler looks like, this was a template handler I made for SAM templates: https://github.com/Sceptre/sceptre-sam-handler.

I'd be happy to talk more about this in person. If you get on our slack channel, we can chat and find some time to maybe do a Google Meet and talk about this.

What do you think about this?

docs/_source/docs/templates.rst Outdated Show resolved Hide resolved
docs/_source/docs/templates.rst Outdated Show resolved Hide resolved
docs/_source/docs/templates.rst Outdated Show resolved Hide resolved

.. code-block:: yaml

Parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if I see a point in including this generated CDK stack template in these docs. CDK stacks are SOOOOOO ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, I thought it was the point to show this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah. This is too much stuff without much gain. I don't think we need to include the sample generated template here. I just don't see what the reader gains from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, removed the generated template

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Co-authored-by: Jon Falkenstein <77296393+jfalkenstein@users.noreply.github.com>
@X-Guardian
Copy link
Contributor Author

I've also created an initial version of a Sceptre CDK template handler, as per your suggestion. You can take a look at it here: Sceptre/sceptre-cdk-handler#1

@jfalkenstein
Copy link
Contributor

I like where you're going and we definitely need to update these docs, but let's hold off on changing these docs until the template handler is available, and then we can reference the CDK handler in the docs rather than including this rather tricky code sample.

@jfalkenstein
Copy link
Contributor

I'm applying the "On Hold" label to this PR until we can get the Template Handler PR merged/deployed. The instructions will be a lot simpler when we can just say "Use this template handler".

@jfalkenstein
Copy link
Contributor

@X-Guardian, the CDK Handler has now been deployed! You can update the docs here to point to it.

@jfalkenstein
Copy link
Contributor

Closing this as a duplicate of #1285

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.

The Sceptre Template AWS CDK Example is out of date and does not support CDK Assets
2 participants