Skip to content

[feature] Add AWS sms client #3134

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

Merged
merged 16 commits into from
Mar 13, 2025
Merged

Conversation

JuJinPark
Copy link
Contributor

@JuJinPark JuJinPark commented Mar 9, 2025

What's changed?

Add AWS sms client
#3111

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Looking for Feedback On:

  • Signature Logic: We might want to make AwsAuthenticationBuilder a public class for reusability—thoughts?(If so, I can add more unit tests for it.)
  • Testing Approach: I plan to add more tests for AwsSmsClientImpl, but I’m unsure about the best approach since I’m not very familiar with the testing conventions or guidelines. Since other SMS implementations don’t have tests, I’ve kept it minimal for now.
  • Translation Review: If anyone can review or refine the Chinese translation in the guide, that’d be much appreciated.

@github-actions github-actions bot added the doc Improvements or additions to documentation label Mar 9, 2025
@JuJinPark JuJinPark changed the title Feature/aws sms [feature] Add AWS sms client Mar 9, 2025
@a-little-fool
Copy link
Contributor

I'm the same. You can take a look at this. According to @yunfan24 , I closed the previous PR and improved it before submitting it again.

@JuJinPark
Copy link
Contributor Author

@a-little-fool You mean I should check the code style and commit message, right?

@a-little-fool
Copy link
Contributor

a-little-fool commented Mar 9, 2025

@a-little-fool你的意思是我应该检查代码风格和提交信息,对吗?

What I mean is that you can first run and check locally (according to the help documentation), so you don't have to push it up and make changes again. This might be more convenient, otherwise it would be like my PR I pushed it many times haha.

@yunfan24
Copy link
Member

yunfan24 commented Mar 9, 2025

Hi, there is also a part in the manager module of the backend that needs to be updated, and corresponding options also need to be added to the frontend if possible. You can refer to this PR #3135.

@yunfan24
Copy link
Member

You can close the already resolved issues by clicking this.
image

@yunfan24
Copy link
Member

LGTM!Thanks

@yunfan24 yunfan24 merged commit 3468739 into apache:master Mar 13, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Improvements or additions to documentation docker docker-compose script webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants