Skip to content

[Improvement-11913] Mask password when creating/editing alert instances#14415

Merged
zhongjiajie merged 2 commits intoapache:devfrom
c3Vu:dev
Jun 28, 2023
Merged

[Improvement-11913] Mask password when creating/editing alert instances#14415
zhongjiajie merged 2 commits intoapache:devfrom
c3Vu:dev

Conversation

@c3Vu
Copy link
Copy Markdown
Contributor

@c3Vu c3Vu commented Jun 27, 2023

Purpose of the pull request

Brief change log

See above

Verify this pull request

image

@EricGao888 EricGao888 added this to the 3.2.0 milestone Jun 28, 2023
@EricGao888 EricGao888 added the improvement make more easy to user or prompt friendly label Jun 28, 2023
EricGao888
EricGao888 previously approved these changes Jun 28, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@EricGao888 EricGao888 left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for CI to pass

@EricGao888 EricGao888 added the first time contributor First-time contributor label Jun 28, 2023
@EricGao888
Copy link
Copy Markdown
Member

EricGao888 commented Jun 28, 2023

@c3Vu Could u plz pull the latest dev code and rebase this PR on dev? I think this will help CI pass.
image

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 28, 2023 •
edited
Loading

Codecov Report

Merging #14415 (f2d43d1) into dev (8a2c05f) will decrease coverage by 0.03%.
The diff coverage is 81.81%.

❗ Current head f2d43d1 differs from pull request most recent head 44f0fdd. Consider uploading reports for the commit 44f0fdd to get more accurate results

@@             Coverage Diff              @@
##                dev   #14415      +/-   ##
============================================
- Coverage     38.51%   38.48%   -0.03%     
+ Complexity     4557     4546      -11     
============================================
  Files          1237     1236       -1     
  Lines         43507    43490      -17     
  Branches       4813     4812       -1     
============================================
- Hits          16756    16737      -19     
  Misses        24898    24898              
- Partials       1853     1855       +2
Impacted Files Coverage Δ
...cheduler/common/constants/DataSourceConstants.java 0.00% <ø> (ø)
.../org/apache/dolphinscheduler/spi/enums/DbType.java 0.00% <0.00%> (ø)
...in/alert/dingtalk/DingTalkAlertChannelFactory.java 98.82% <100.00%> (+0.01%) ⬆️
...r/plugin/alert/email/EmailAlertChannelFactory.java 98.85% <100.00%> (+0.01%) ⬆️
...plugin/alert/feishu/FeiShuAlertChannelFactory.java 97.56% <100.00%> (+0.06%) ⬆️
...in/alert/telegram/TelegramAlertChannelFactory.java 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Jun 28, 2023 •
edited
Loading

this dead link will be fix in #14406 but it seem 14406 is also not ready to merge

zhongjiajie
zhongjiajie reviewed Jun 28, 2023
View reviewed changes
...mail/src/test/java/org/apache/dolphinscheduler/plugin/alert/email/EmailAlertChannelTest.java Outdated
PasswordParam mailPassword = PasswordParam.newBuilder(MailParamsConstants.NAME_MAIL_PASSWD, "passwd")
InputParam mailPassword = InputParam.newBuilder(MailParamsConstants.NAME_MAIL_PASSWD, "passwd")
.setPlaceholder("if enable use authentication, you need input password")
.setValue("escheduler123")
Copy link
Copy Markdown
Member

@zhongjiajie zhongjiajie Jun 28, 2023

Choose a reason for hiding this comment

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

does escheduler123 is a default value? if also we should remove it or change to dolphinscheduler

I guess escheduler is for easy scheduler is pervious name of dolphinscheduler

@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Jun 28, 2023

approval ci run

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jun 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@zhongjiajie zhongjiajie merged commit 50195c4 into apache:dev Jun 28, 2023
@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Jun 28, 2023

Hi @c3Vu , it is a great step for you to become part of dolphionscheduler contributors community 🎉 .
If you want to continue your contribute but could not find issues, maybe you could start in https://github.com/apache/dolphinscheduler/contribute
or just search our issue list https://github.com/apache/dolphinscheduler/issues, Looking for something you interesting.

IT-Kwj pushed a commit to IT-Kwj/dolphinscheduler that referenced this pull request Jul 14, 2023
@c3Vu
[Improvement-11913] Mask password when creating/editing alert instanc…
df8265e 
…es (apache#14415)

* [Improvement-11913] Mask password when creating/editing alert instances
zhongjiajie pushed a commit that referenced this pull request Jul 20, 2023
@c3Vu @zhongjiajie
[Improvement-11913] Mask password when creating/editing alert instanc…
774d61d 
…es (#14415)

* [Improvement-11913] Mask password when creating/editing alert instances

(cherry picked from commit 50195c4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhongjiajie zhongjiajie zhongjiajie approved these changes

@EricGao888 EricGao888 EricGao888 left review comments

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@kezhenxu94 kezhenxu94 Awaiting requested review from kezhenxu94

Assignees

@c3Vu c3Vu

Labels

backend first time contributor First-time contributor improvement make more easy to user or prompt friendly ready-to-merge

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

[Improvement] [Alert] should mask the password of email password

4 participants

@c3Vu @EricGao888 @codecov-commenter @zhongjiajie