Skip to content

[Fix] Executing install.sh failed when ${password} and others contain @ - #6733

Closed
tracehh wants to merge 0 commit into
apache:devfrom
tracehh:dev
Closed

[Fix] Executing install.sh failed when ${password} and others contain @#6733
tracehh wants to merge 0 commit into
apache:devfrom
tracehh:dev

Conversation

@tracehh

@tracehh tracehh commented Nov 8, 2021

Copy link
Copy Markdown
Contributor

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

@tracehh

tracehh commented Nov 8, 2021

Copy link
Copy Markdown
Contributor Author
  1. sed -i ${txt} "s@^spring.datasource.password=.*@spring.datasource.password=${password}@g" conf/datasource.properties
  2. ${password} is qwer@1234
    Then one more @

while sed -i ${txt} "s/^spring.datasource.password=.*/spring.datasource.password=${password}/g" and ${password} is qwer/1234, the same error

May have a better way

@codecov-commenter

codecov-commenter commented Nov 8, 2021

Copy link
Copy Markdown

Codecov Report

Merging #6733 (ab7e7ad) into dev (24bd8a1) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #6733      +/-   ##
============================================
- Coverage     37.99%   37.93%   -0.06%     
+ Complexity     3243     3235       -8     
============================================
  Files           643      643              
  Lines         26409    26409              
  Branches       2861     2861              
============================================
- Hits          10034    10019      -15     
- Misses        15453    15469      +16     
+ Partials        922      921       -1     
Impacted Files Coverage Δ
...he/dolphinscheduler/common/enums/SqoopJobType.java 0.00% <0.00%> (-88.89%) ⬇️
...org/apache/dolphinscheduler/remote/utils/Host.java 37.77% <0.00%> (-2.23%) ⬇️
...inscheduler/common/task/sqoop/SqoopParameters.java 74.00% <0.00%> (-2.00%) ⬇️
...inscheduler/common/thread/ThreadPoolExecutors.java 19.23% <0.00%> (-1.93%) ⬇️
...dolphinscheduler/remote/future/ResponseFuture.java 81.35% <0.00%> (-1.70%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 52.11% <0.00%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24bd8a1...ab7e7ad. Read the comment docs.

Comment thread install.sh Outdated
sed -i ${txt} "s@^spring.datasource.username=.*@spring.datasource.username=${username}@g" conf/datasource.properties
sed -i ${txt} "s@^spring.datasource.password=.*@spring.datasource.password=${password}@g" conf/datasource.properties
sed -i ${txt} "s/^spring.datasource.username=.*/spring.datasource.username=${username}/g" conf/datasource.properties
sed -i ${txt} "s/^spring.datasource.password=.*/spring.datasource.password=${password}/g" conf/datasource.properties

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch! But when we password contains /, does this command work?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think maybe we should find more general way to escape specific characters

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah,need to consider both of 's','g','@'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we have to consider other character that must be escaped. maybe we should add a escape function to do that, WDYT? @tracehh

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes,I am working on this

@lenboo

lenboo commented Nov 10, 2021

Copy link
Copy Markdown
Contributor

@tracehh Is there an issue associated with this PR

@zhongjiajie

Copy link
Copy Markdown
Member

@tracehh Is there an issue associated with this PR

@tracehh Yeah, maybe you should create a issue and target PR to it.

@sonarqubecloud

Copy link
Copy Markdown

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@tracehh

tracehh commented Nov 11, 2021

Copy link
Copy Markdown
Contributor Author

ok, I create an issue : #6781

I open another pr for this issue. @zhongjiajie

@zhongjiajie

Copy link
Copy Markdown
Member

ok, I create an issue : #6781

I open another pr for this issue. @zhongjiajie

@tracehh Hi, you could just edit PR describe instead of open another PR, cause another PR would lost context about preview review.
I could reopen PR and edit describe like screenshot belove

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants