feat: add synth & kwargs #288
Conversation
| ) -> str: | ||
| synthesize: bool = False, | ||
| **kwargs: Dict[str, Any], | ||
| ) -> Optional[str]: |
There was a problem hiding this comment.
I know this function doesn't have a docstring right now, but I think it would be good to add one while we're here.
I'm skeptical on using kwargs here -- it is two extra optional parameters, but they're completely undocumented. Why not just add role_prefix and policy_prefix?
There was a problem hiding this comment.
This is intended. kwargs is an escape hatch and would allow us to pass any other parameters from Seed-Farmer (or any other tool) in the future without breaking backward-compatibility.
There was a problem hiding this comment.
For what value? To decouple seed-farmer and code-seeder versions slightly?
| @@ -96,7 +100,7 @@ def deploy_seedkit( | |||
| if stack_exists: | |||
| deploy_id = stack_outputs.get("DeployId") | |||
| LOGGER.info("Seedkit found with DeployId: %s", deploy_id) | |||
| template_filename: str = _cfn_seedkit.synth( | |||
| template_filename: Optional[str] = _cfn_seedkit.synth( | |||
There was a problem hiding this comment.
Would it make more sense to have synth return the string template instead of the path?
Right now there's a coupling that synthesize=False changes the output value. That looks like quite a refactor though, so maybe it isn't worth it. Probably just add an assertion under if not synthesize that template_filename is non-None?
There was a problem hiding this comment.
Yeah I considered refactoring this but did not want to make too intrusive changes. Good call about an assertion though.
| file.write(template) | ||
| return output_filename | ||
| else: | ||
| sys.stdout.write(template) |
There was a problem hiding this comment.
Will this get mixed in with log output? Is that OK?
There was a problem hiding this comment.
Redirecting this into a file i.e:
codeseeder deploy seedkit test --synth > test.yaml
Doesn't mix it up with the log output and produces a valid template
Issue #, if available: #272
Description of changes:
--synthoption todeploy seedkitCLI. This will synthesize and output the seedkit template but not deploy it, to allow users to change it and deploy outside of codeseeder if required. This will allow to support customisation requirements such as adding tags, changing IAM paths or resource naming conventions.**kwargsescape hatch todeploy_seedkitAPI and support setting IAM paths via:By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.