Skip to content

chore(cognito-identitypool): refactor and remove Digits authentication provider#31482

Merged
mergify[bot] merged 15 commits intoaws:mainfrom
Leo10Gama:cog-prototype
Sep 26, 2024
Merged

chore(cognito-identitypool): refactor and remove Digits authentication provider#31482
mergify[bot] merged 15 commits intoaws:mainfrom
Leo10Gama:cog-prototype

Conversation

@Leo10Gama
Copy link
Copy Markdown
Contributor

@Leo10Gama Leo10Gama commented Sep 18, 2024 •
edited by comcalvi
Loading

Issue # (if applicable)

N/A

Reason for this change

The authentication providers and their logic in this module have bad code smells, and thus, we have refactored them to bring the module more in line with CDK standards and best practices. In addition, the Digits authentication provider has been deprecated since September 2017, so it has been removed.

Description of changes

  • Any modules relating to the Digits auth have been removed, as the service itself is deprecated.
  • The IdentityPoolProviders and IdentityPoolAuthenticationProviders interfaces have been merged, as there did not seem to be a reason to keep them separate, aside from differentiating third-party and internal providers.
  • Some grammar, punctuation, formatting, and capitalization changes

Description of how you validated changes

Unit tests and integration tests have been tweaked only as necessary to confirm these changes. Since they all still pass or show no need to be updated, we can confirm that this refactor does not affect them. The integration test has also been updated to reflect that the previous Google prop for clientSecret is deprecated, and use clientSecretValue instead.

BREAKING CHANGE: The IdentityPoolProviderType.DIGITS and IdentityPoolProviderUrl.DIGITS enum values, and IdentityPoolDigitsLoginProvider interface have been removed, as well as the digits attribute of the IdentityPoolAuthenticationProviders interface.

Checklist

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

@github-actions github-actions bot added the p2 label Sep 18, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team September 18, 2024 17:29
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 18, 2024
aws-cdk-automation
aws-cdk-automation previously requested changes Sep 18, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@Leo10Gama
Copy link
Copy Markdown
Contributor Author

Leo10Gama commented Sep 18, 2024

Exemption Request - this is a refactor; seeing no changes to the integ tests is expected

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 18, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 19, 2024 20:07

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

shikha372
shikha372 reviewed Sep 20, 2024
View reviewed changes
@Leo10Gama @shikha372
Update packages/@aws-cdk/aws-cognito-identitypool-alpha/lib/identityp…
7ca1fd6 
…ool.ts

Co-authored-by: Shikha Aggarwal <shikhaaggarwal1995@gmail.com>
@shikha372 shikha372 self-assigned this Sep 20, 2024
comcalvi
comcalvi suggested changes Sep 23, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

I don't think merging is worth it...we should ask customers to specify the name of the thing that they need to, not some arbitrary value that makes our lives easier, but not theirs.

@Leo10Gama Leo10Gama marked this pull request as ready for review September 24, 2024 19:55
aws-cdk-automation
aws-cdk-automation previously requested changes Sep 24, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@Leo10Gama
Copy link
Copy Markdown
Contributor Author

Leo10Gama commented Sep 24, 2024

Will update the integ test, since trying to cdk deploy it (instead of yarn integ running it) gives the error:

[WARNING] aws-cdk-lib.aws_cognito.UserPoolIdentityProviderGoogleProps#clientSecret is deprecated.
  use clientSecretValue instead
  This API will be removed in the next major release.

@aws-cdk-automation aws-cdk-automation dismissed their stale review September 24, 2024 23:25

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@Leo10Gama Leo10Gama changed the title fix(cognito-identitypool): refactor authentication providers chore(cognito-identitypool): refactor and remove Digits authentication provider Sep 26, 2024
@comcalvi comcalvi self-assigned this Sep 26, 2024
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 26, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@Leo10Gama
Copy link
Copy Markdown
Contributor Author

Leo10Gama commented Sep 26, 2024

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 26, 2024

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/codeql.yml without workflows permission

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 26, 2024

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 5b5c7c4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 26, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit a633ee6 into aws:main Sep 26, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 26, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2024
@Leo10Gama Leo10Gama deleted the cog-prototype branch September 26, 2024 18:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

+2 more reviewers

@shikha372 shikha372 shikha372 approved these changes

@comcalvi comcalvi comcalvi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@comcalvi comcalvi

Labels

contribution/core This is a PR that came from AWS. p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Leo10Gama @aws-cdk-automation @shikha372 @comcalvi