feat: replace role map lambda with cfn code #12231
Conversation
danielleadams
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
danielleadams
force-pushed
the
feat/replace-role-map-lambda
branch
from
a1d707f to
0de6726
Compare
danielleadams
force-pushed
the
feat/replace-role-map-lambda
branch
2 times, most recently
from
91be801 to
0d17642
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
danielleadams
force-pushed
the
feat/replace-role-map-lambda
branch
from
aa5f3c0 to
0df0656
Compare
...on-tests/src/__tests__/migration_tests_v10/__snapshots__/auth-add-all.migration.test.ts.snap
Outdated
Show resolved
Hide resolved
danielleadams
added
the
do-not-merge
| } catch (e) { | ||
| console.log(e); | ||
| } |
There was a problem hiding this comment.
I noticed this is a pattern in this file. But is this a good practice?
What happens at the caller in this case? Does it handle undefined response gracefully?
What does undef response mean for the caller ? If it's handled gracefully does this mean CLI would produce different outcomes if transient error happen?
I think we should either have retry loop with exponential backoff OR such behavior should be configured when we spin up CongnitoIdentity client. In either case transient errors. if they persist, should propagate and stop user's command.
There was a problem hiding this comment.
I noticed this is a pattern in this file. But is this a good practice?
I don't think this is a good pattern because it swallows the error and does not surface errors that arise from SDK, including on errors from the caller payload. I also don't want this pattern to leak into the production code where it's called from the CLI.
What does undef response mean for the caller ?
Since these are test helpers, I imagine the desired/expected behavior would be that the test fails, but again, we aren't surfacing the error, so there isn't a good understanding of why the test failed which can cause more pain.
If it's handled gracefully does this mean CLI would produce different outcomes if transient error happen?
I don't know if this is relevant because this is just the test calling straight to the SDK (or at least that's how I am using it) and not going through CLI.
I think we should either have retry loop with exponential backoff OR such behavior should be configured when we spin up CongnitoIdentity client. In either case transient errors. if they persist, should propagate and stop user's command.
I think we can do this, but I think this is out of scope for this PR. I also am not sure if failed calls to SDK are a huge pain point in tests. I would think that the issues with incorrect payloads and/or failing to match SDK responses cause test failures, which wouldn't be solved by retries.
There was a problem hiding this comment.
woah, I didn't notice this is tests. nvm me.
There was a problem hiding this comment.
But fwiw. I'd just remove try catch and let tests fail fast.
|
I didn't see any indication that we've tested creating/migrating these resources in headless mode which is important for Studio's integration. Can we add a test for this? |
|
There's been follow up tasks to write e2e headless tests |
Description of changes
This removes the Lambda callout custom resource that creates the role mapping for the identity pool, and adds it to the CFN code for the identity pool role attachments. The Lambda callout only runs when a user pool group is created, so the role attachment is only added to the CFN code with a user pool group.
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.