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

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

Merged
merged 15 commits into from
Sep 26, 2024

Conversation

Leo10Gama
Copy link
Member

@Leo10Gama Leo10Gama commented Sep 18, 2024

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
Copy link
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
Member Author

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 self-assigned this Sep 20, 2024
Copy link
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.

packages/@aws-cdk/aws-cognito-identitypool-alpha/README.md Outdated Show resolved Hide resolved
@Leo10Gama Leo10Gama marked this pull request as ready for review September 24, 2024 19:55
Copy link
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
Member Author

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
Copy link
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
Member Author

@Mergifyio update

Copy link
Contributor

mergify bot commented Sep 26, 2024

update

❌ Mergify doesn't have permission to update

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
Collaborator

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

Copy link
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
12 checks passed
Copy link

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.
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
Development

Successfully merging this pull request may close these issues.

4 participants