-
Notifications
You must be signed in to change notification settings - Fork 4
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
Supporting non-python CDK projects #10
Conversation
…dler-refactor-p2-example
…eptre-cdk-handler into jf/handler-refactor-p3-readme
…ndler into jf/non-python-cdk
Co-authored-by: Simon Heather <32168619+X-Guardian@users.noreply.github.com>
@X-Guardian I've resolved the validation errors you've pointed out and have even buried the useless stdout and stderr results from when we check if the commands exist. I've also explicitly caught the CalledProcessError that gets raised when running subprocesses and am re-raising them wrapped in a SceptreException. This should hide the python stacktrace part (that isn't useful except for the actual message itself). It will not hide the JavaScript stacktrace, though. And that's good, because that's ultimately the source of the issue. I believe this should cover all the concerns you've raised. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent work @jfalkenstein! I've looked thru the code and it generally looks good to me. I just have a few suggestions but nothing that would block approval. It looks like there are a few alternative ways to setup sceptre to deploy cdk apps. I have only tested sceptre with python (without cdk.json) and it looks like it's generally working. There are of course some scenarios that I think need improving however I think it can be handled as follow on changes. This PR is pretty large as is so I recommend we merge and create issues to handle with follow on PRs.
sceptre_cdk_handler/cdk.py
Outdated
"You must specify the stack_logical_id of the stack in your app in order to use the " | ||
"use a cdk.json file with the CDK handler." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @X-Guardian here. your sentence has some extra words, it reads like this ... in your app in order to use the use a cdk.json file with the CDK handler
@@ -0,0 +1,31 @@ | |||
# This template is an example of using the CDK template handler to deploy with a stack from a CDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mixing in this example with the existing python example makes things sort of confusing. I think it would clear things up if we could put the two in completely separate folders. Another option is to provide a tree list of the sceptre-example folder and provide an overview of the file relationships
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zaro0508, Having this example here demonstrates how using Sceptre can tie together CDK projects and other resources into an environment. I don't see how this makes things confusing. Are you able to elaborate on why you find this confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's confusing because files for a non-python project are mixed in with files for a python example. Also python are put in the sceptre standard templates
folder while non-python files are not. Those are all fine except i think new users looking at this handler would be confused if they just looked at sceptre-example
folder for the first time.
"You cannot use nested values within your CDK context when your path points to a cdk.json " | ||
"file. If you need to specify such values, put them in the context of your cdk.json " | ||
"file." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i find this confusing. It's saying you cannot use nested values in cdk.json but it also says to use cdk.json if you need to specify such values. I assume such values
refers to nested values?
The sentence in the readme is pretty clear..
Complex data structures (i.e. lists and dicts) are not supported in your CDK context from Sceptre.
You can explicitly set those up on your cdk.json (if they're required), but the CDK template handler
will only accept simple key/value pairs for your context that it passes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I'd can repeat that phrasing here.
# project instead of a python file. The CDK Handler is smart enough to synthesize the template and | ||
# publish any assets using the CDK CLI. This lets the handler support all CDK-supported languages | ||
# rather than only Python. | ||
path: ../cdk-app/cdk.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i assume we do this because cdk.json is on in templates
folder? why not setup this example within the scpetre folder convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's because the cdk.json is outside the templates directory. Since a CDK project can be a lot more substantial than a single template file, it's likely users of this handler would want to place their CDK project outside of templates
. In several senses, a CDK project is not a "template" and it doesn't make so much sense to bury a CDK project within a directory named templates
. In fact, with the way that CDK initializes repositories and such, it generally makes more sense to have the CDK project be in a different directory than the Sceptre stuff.
I could put a comment to this effect here to clear up the confusion, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, users would probably be confused without a little explanation.
I am receiving the following message when I launch my
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't spot anything troublesome, but I'm honestly not familiar yet with a lot of what's happening in these modules.
@X-Guardian I'm not sure how that would be happening. Both an empty_dict and None resolve to falsy in Python. This line of code is intended to only make that warning show up when |
Ok, I worked out what was causing this. It is a Sceptre feature/bug whereby if you have for example a |
@X-Guardian is it possible that you have defined |
Nope. Try it yourself. Copy the |
@X-Guardian Are you saying you have two files of the same name, one in a parent StackGroup and one in a child StackGroup? |
Alright, I think this PR has gotten a lot of comments and reviews. Thank you all very much @zaro0508 , @ericabrauer , @X-Guardian , @dboitnot. Since I've addressed/resolved all requested changes and the only lingering issue seems like, if it's an issue at all, it's an issue with Sceptre itself and not this handler, I'm going to get this out! |
Co-authored-by: Simon Heather <32168619+X-Guardian@users.noreply.github.com>
This change adds support for referencing a
cdk.json
as the Template Handler'spath
, enabling support for CDK infrastructure that isn't even defined in Python!