feat: Add VPC/subnets/sg config for multi-account project template to sagemaker-templates-service-catalog module
#35
Conversation
...sagemaker/sagemaker-templates-service-catalog/templates/multi_account_basic/product_stack.py
Show resolved
Hide resolved
Added vpc and subnetid parameters.
added vpcid and subnetid parameters
Added vpcID, subnetID parameters for preprod and prod accounts. They are mandatory as of now
Added vpc and subnet parameters for prod and preprod. They are mandatory as of now
added vpc and subnetid parameters
...rvice-catalog/templates/multi_account_basic/pipeline_constructs/deploy_pipeline_construct.py
Outdated
Show resolved
Hide resolved
… lists. Removed all parameters as cfnparameters
… lists. Removed all parameters as cfnparameters
- standartize parameters - naming conventions & housekeeping Signed-off-by: Anton Kukushkin <[email protected]>
kukushking
changed the title
sagemaker-templates-service-catalog module
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
| preprod_account_id = CfnParameter( | ||
| self, | ||
| "PreprodAccountId", | ||
| type="String", | ||
| description="Pre-prod account id.", | ||
| ).value_as_string |
There was a problem hiding this comment.
Could we still include these parameters, only offering default values? This would allow the template user to overwrite these value.
There was a problem hiding this comment.
Yes, we can pass down the default parameter values from the props of the module - this is how it was few commits back... I thought we can hide this setup from data scientists completely but I agree there still might be a use case where this may need to be changed.
One consequence is that if the users want no VPC in dev, they would still have to provide some dummy value since there are no optional parameters in CFN.
Since we're here: another change I've been contemplating is possibly moving endpoint configs out as seed-farmer data files. What do you think?
There was a problem hiding this comment.
My view is we should leave it without the parameters. Someone using the template to create a project might not know these values and can lose interest using the template as they feel it is complex to use
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
Signed-off-by: Anton Kukushkin <[email protected]>
Describe your changes
Issue ticket number and link
Checklist before requesting a review
CHANGELOG.MDwith a description of my changesscripts/validate.sh)seedfarmer applyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.