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

optionally allow asset publishing #38

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ahmedkhattab
Copy link
Contributor

flag enable_asset_publishing is used to control whether to add an optional step to publish cdk assets to s3, this is useful if the target code repository makes use of assets (for example lambda function code as an asset)

Can be further improved by:

  • looping over assets manifests (for now the 3 asset manifests for 3 environments are hardcoded)
  • externalising this logic to a separate script/construct to be reusable in any pipeline construct

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

flag `enable_asset_publishing` is used to control whether to add an optional step to publish cdk assets to s3, this is useful if the target code repository makes use of assets (for example lambda function code as an asset)

Can be further improved by:
- looping over assets manifests (for now the 3 asset manifests for 3 environments are hardcoded) 
- externalising this logic to a separate script/construct to be reusable in any pipeline construct
Copy link
Contributor

@Fatema Fatema left a comment

Choose a reason for hiding this comment

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

Just minor comments before approving this request

@@ -45,6 +45,7 @@ def __init__(
preprod_account: int,
prod_account: int,
deployment_region: str,
enable_asset_publishing: bool = False, # set to true if you would like to enable an asset publishing stage
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth having this be controlled by a CFN Parameter and be part of the template itself, this will allow the creation of projects with asset publishing enabled and disabled as desired by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean to expose it all the way up on the service catalog product level ?
If so i think it will create some pain since we won't be able to easily toggle the creation of the resources based on the parameter value ( it won't be a straight forward if statement and we will need to use CfnConditions, something like this: https://loige.co/create-resources-conditionally-with-cdk/ ) or may be i am mistaken, let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

yh or can also be a condition inside the codebuild as workaround as well this is just to ensure no error arise when running the code with no assets to publish.

Copy link
Contributor Author

@ahmedkhattab ahmedkhattab left a comment

Choose a reason for hiding this comment

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

@Fatema how about this ?
so now we have: the overall controlling flag which has a default value of false, then these if conditions which makes sure the build won't fail even if no assets are to be published

@Fatema
Copy link
Contributor

Fatema commented Aug 25, 2022

This looks good! please pull latest changes then will merge this to the main branch :)

Fatema and others added 7 commits August 25, 2022 16:00
* adding template for custom image handling

* adding byoc setup

* fixed script

* adding more seed code and upating the docker build script

* fixes to typos and iam

* fix some errors in the code and added a read me for advanced topics

* byoc changes

* renamed folder name to mlops_sm_project_template

* modified description

* fixes to BYOC seed code

* reformatting and adding more docs

* more docs

* more docs

* first commit

* minor bug fixes

Co-authored-by: Fatema Alkhanaizi <[email protected]>
Co-authored-by: Georgios Schinas <[email protected]>
flag `enable_asset_publishing` is used to control whether to add an optional step to publish cdk assets to s3, this is useful if the target code repository makes use of assets (for example lambda function code as an asset)

Can be further improved by:
- looping over assets manifests (for now the 3 asset manifests for 3 environments are hardcoded) 
- externalising this logic to a separate script/construct to be reusable in any pipeline construct
@ahmedkhattab
Copy link
Contributor Author

changes pulled for this pr and the other one with the policies change
Thanks you!

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.

4 participants