Feature/air 191 - #37
Conversation
|
@is-achakraborty can you add is-wschumacher and |
is-wschumacher
left a comment
is-wschumacher
left a comment
There was a problem hiding this comment.
This comment may or may not be outside of the scope of this review. Generally I would request that the README be updated to reflect the changes made here, however, I note that it looks like the existing actions and README have already diverged, so getting those back in alignment may be a larger task better suited for a follow up review.
c7fd92a to
82aa1f9
Compare
|
@is-achakraborty, Going forward please create commit messages starting with the JIRA ticket number: the commit message. Please follow the same for feature branches and PR creation. For example Branch name could be something like This will ensure that the commits, branches and the PR's along with their current status are being tracked on the corresponding JIRA ticket. |
| @@ -0,0 +1,14 @@ | |||
| # This file is the sharable action with build,unittest, integration test, code coverage reports. | |||
There was a problem hiding this comment.
please remove this function out. we don't want to have infra provisioning is performed from GitHub Action
There was a problem hiding this comment.
We need to use serverless setup to deploy lambda. I discussed this with @johnseq-iterative and clearly mentioned that infrastructure provisioning code should not be part of this pipeline. If you see the serverless templates here only the lambda specific roles and events are getting created from the template. We are expecting the other infra should already be in place before we run this pipeline.
There was a problem hiding this comment.
@is-ttang Avik and I did discuss this. My understanding is the plan is to keep infra+code provisioning together until we are ready to separate them out. That keeps the CICD responsibility squarely on one team while we wait to e.g. move to gitlab. Does that work as an interim solution?
There was a problem hiding this comment.
Sorry -- reading this more closely. @is-achakraborty this makes sense. We'll split out as much as we can from our templates and create out of band. That's fine.
There was a problem hiding this comment.
Yes. but, you can run the SLS provisioning separately. it requires have more privilege for GitHub to access AWS to provision. Second, you just need to run ONCE, NOT run lambda update every time. At moment, GitHub OpenID connect scope is locked down to ECR publish and Lambda S3 write
There was a problem hiding this comment.
Separately means out of the pipeline? If yes then I think no meaning in using this pipeline anymore.
Yes we need the required AWS permissions for the pipeline to work. We can sit together and decide on the minimal subset of permission required.
| - name: Update container lambda - get image uri with given image tag | ||
| - name: Deploy lambda | ||
| if: ${{ steps.app_settings.outputs.app_registry == 'ecr' }} |
There was a problem hiding this comment.
this step is needed to deploy lambda container
There was a problem hiding this comment.
Ok, then I need to distinguish the serverless deployment path. I am going to add an app parameter say deployer: sls in the .cicd-profile.yaml for this purpose. I would make sure that if some one not defines that param in their project then it would take some default value
| aws_region: | ||
| description: aws region | ||
| required: true | ||
| aws_account_id: |
There was a problem hiding this comment.
this change is to break the pipeline in training-pipeline
There was a problem hiding this comment.
Can you give me the repo reference where it is getting used
There was a problem hiding this comment.
could you make NOT require? so NOT require to make any change in the training-pipeline (repo)
There was a problem hiding this comment.
04ba930 to
18252df
Compare
608c357 to
382bd53
Compare
6020768 to
f6e3610
Compare
f6e3610 to
f495eff
Compare
| run: | | ||
| echo "push zip..." | ||
| aws s3 cp ${{ inputs.repository }}.zip ${{ inputs.registry }}/${{ inputs.env }}/${{ inputs.repository }}.zip | ||
| # aws s3 cp ${{ inputs.repository }}.zip ${{ inputs.registry }}/${{ inputs.env }}/${{ inputs.repository }}.zip |
There was a problem hiding this comment.
why remove s3 cp. this action is push Lambda zip
There was a problem hiding this comment.
We are not using this step. sls would take care of building the zip and pushing that to s3.
5 participants
No description provided.