fix: Correct CognitoUserPool SmsConfiguration validation type

[lafiosca]

Issue #, if available: #1252

Description of changes: Change CognitoUserPool SmsConfiguration type validation from "list of dict" to just "dict", to match what CloudFormation expects.

Description of how you validated changes: Tested locally with bin/sam-translate.py.

Checklist:

• Write/update tests
• make pr passes
• Update documentation
• Verify transformed template deploys and application functions as expected
• Add/update example to examples/2016-10-31

I don't think the documentation or examples need to be updated because this fix is not even for a feature of SAM. In fact, it makes me wonder why SAM is performing this redundant validation which can become out of sync with CloudFormation.

I actually did try updating tests/translator/input/cognito_userpool_with_event.yaml to include an SmsConfiguration property. I was able to run it through bin/sam-translate.py as the Development Guide specifies, but when I tried to store the resulting output as tests/translator/output/cognito_userpool_with_event.json and run the test suite again, it didn't work correctly. I'm not sure what else needs to be done to make that work, but again it's not even clear to me if this is what SAM should be doing.

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

[jfuss]

@lafiosca Thanks for the fix. Can you add a simple test to validate this? This will help insure this isn't regressed.

[lafiosca]

@jfuss I attempted to build a test to include with the original PR, but it would not pass. I think I am misunderstanding something about how the test suite works. Please see the original post above for more details about what I tried if you think you can help me get there.

[lukrizal]

Hi @jfuss and @lafiosca any updates? Looking forward to this.

[lafiosca]

Sorry @lukaserat it looks like the maintainers are waiting on me to add a unit test for this. I tried to before I submitted the PR, but I couldn't get it to work and didn't understand how to set it up properly. I don't have the time available to dig in deeply without additional instruction.

[BertCasteel]

Any update or roadmap @jfuss? I was really hoping to integrate cognito with my serverless app.

[YAMABASS80]

Hi, this is critical for me. I wrote bunch of codes with SAM but because of this I can't get my work done!!!

[lafiosca]

@YAMABASS80 I'd suggest opening a new issue as a copy of #1252 and linking it here ... The creator of that issue closed it without explaining why.

[Raaghu]

this changes is OK ,
This needs to be merged to unblock using sam for deploying cognito userpool with phone_number as alias

[Raaghu]

WorkAround until this gets fixed is to create a UserPool in a Nested Stack (child stack should not have AWS::Serverless-2016-10-31 Transformation)

[lafiosca]

My own workaround was to run the forked version of the script locally.

[ShadowBearVR]

My own workaround was to run the forked version of the script locally.

@lafiosca What was your process to do this?

[lafiosca]

Sorry @snowhalo it's been a while, so I can't remember specifically... but I know I followed the instructions here to set up for development: https://github.com/aws/serverless-application-model/blob/develop/DEVELOPMENT_GUIDE.md

From there it should be just cloning my PR branch and running the script with python manually.

[ShadowBearVR]

@lafiosca Thanks for the quick reply! I am currently following the instructions here (https://github.com/aws/aws-sam-cli/blob/develop/DEVELOPMENT_GUIDE.md) to compile the CLI (and via step 5 use a different version of the SAM Translator). However I still get the SMS Configuration validation error when I call “samdev deploy”.

[lafiosca]

I'm not sure about samdev deploy, but from my old scripts it looks like I manually ran bin/sam-translate.py

[ShadowBearVR]

@lafiosca Thanks. That almost worked... Unfortunately I am currently relying upon SAM to translate local directories into an S3 bucket via CodeUri, which is the one known limitation of this script. I just need MFA in a SAM App - I didn't expect such a basic requirement to be so much trouble.

[lafiosca]

@snowhalo If you're actively running into difficulty with this, I strongly encourage you to open a new issue essentially copying #1252 and linking here, while the problem is fresh in your mind. I haven't done it myself because I haven't touched this part of my infrastructure in a long time.

[jfuss]

I am going through our PR queue and this still seems like an issue but not super clear. If this is still an issue, I would like to get this through and release. This seems like SAM is just broken for this use-case.

It is not clear why SAM is doing this validation (as @lafiosca pointed out). I am not familiar with what this is for at the moment, so not sure how/if we can remove this completely. The current thinking of the team is not to validate things CloudFormation or the APIs do. There are exceptions to provide a better experience but this doesn't seem like one of them.

Assigning to myself so I can track this and come back to if after I get a feeling of what is in the PR queue.

[lafiosca]

Thanks @jfuss! I haven't looked at this recently, as I haven't had the need to change my long-running deployment where this was an issue. I assume though that it is still an issue, and my only workaround was to run my locally modified version of the repo, which I haven't been maintaining. I would love to see SAM pass the validation responsibility over to CloudFormation for this though. Good luck investigating!

[jfuss]

@lafiosca Given that, I think what I would like to do is close this PR and I will create a tracking issue (or reopen the closed one). I am trying to get the PR queue across repos we own cleaned up and make sure things are actionable.

I won't close this right way, in case I find time to do this now (before the holiday).

[dbenader]

@jfuss has this fix been abandoned?

[dbenader]

@lafiosca Given that, I think what I would like to do is close this PR and I will create a tracking issue (or reopen the closed one). I am trying to get the PR queue across repos we own cleaned up and make sure things are actionable.

I won't close this right way, in case I find time to do this now (before the holiday).

@jfuss any plans to merge this PR? I would really like to be able to create a cognito user-pool with sms configured in the same template file