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

fix: cognito trigger template permissions #11482

Merged
merged 26 commits into from
Dec 7, 2022

Conversation

akshbhu
Copy link
Contributor

@akshbhu akshbhu commented Nov 28, 2022

Description of changes

  • Previously Cognito trigger permissions were missing from auth trigger template, this PR adds those permissions to the template
  • added physical resource id on custom resource lambda function which enables auth trigger.

Issue #7582

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@akshbhu akshbhu requested a review from a team as a code owner November 28, 2022 19:15
@jhockett jhockett changed the title fix: fixing cognito trigger template permissions fix: cognito trigger template permissions Nov 28, 2022
edwardfoyle
edwardfoyle previously approved these changes Dec 6, 2022
Copy link
Contributor

@edwardfoyle edwardfoyle left a comment

Choose a reason for hiding this comment

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

asked a couple small clarifications but if they are all irrelevant then LGTM

jhockett
jhockett previously approved these changes Dec 7, 2022
packages/amplify-category-auth/src/index.js Show resolved Hide resolved
@akshbhu akshbhu merged commit 5d606e0 into aws-amplify:dev Dec 7, 2022
@guidovalente
Copy link

guidovalente commented Jan 7, 2023

just for the record - I had to update a stack today that includes a cognito trigger linked to a lambda function (it's a customMessage trigger). The last changes I had made to the app were under v10.5

as I went to update the stack today, the deployment process failed when getting to the AuthTriggerCustomLambdaStack resource. Specificaly, I was getting an error saying Output 'LambdaExecutionRoleArn' not found in stack '<edited: ARN for my customMessage stack here>'.

What I did was remove the trigger from the template and added it again from scratch, reconfiguring environment variables for that lambda.

It seems these kinds of changes are breaking changes that stop deployments alltogether. This was a small function in my stack that didn't take long to edit, but it could have been a more complicated issue if the lambda had been tied to other resources in a different way.

Is there a preferred way to approach this kind of change in Amplify without having to re-create resources? Or is there a way to force the online deployment to use an older version of the CLI so that I don't have to update my stack?

from what I could see, what happens is that the new Amplify version expects an output in the stack that wasn't there before, so maybe a version change like this should scan for these changes and try to incorporate them into the template?

I'm not an expert with Amplify so please point me in the right direction if I'm not asking the right questions, but at times I feel Amplify is getting in my way when I come across these things and I might just be better off going with raw CF or CDK.

It may just be that Amplify needs a bit more time to evolve, but not being able to deploy a small change to my stack and forcing me to update the CLI version just to be able to add in a few new lines of code to a lambda frustrates me a little, especially when the lambda I wanted to edit was a completely different one unrelated to the trigger.

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.

7 participants