Skip to content

[improverment]:The install.sh script does not check whether the user has sudo privileges#11018

Closed
ZYJBigData wants to merge 3 commits intoapache:devfrom
ZYJBigData:improvement-11006
Closed

[improverment]:The install.sh script does not check whether the user has sudo privileges#11018
ZYJBigData wants to merge 3 commits intoapache:devfrom
ZYJBigData:improvement-11006

Conversation

@ZYJBigData
Copy link

@ZYJBigData ZYJBigData commented Jul 17, 2022

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:

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022

Codecov Report

Merging #11018 (c1a3c3e) into dev (083ab2b) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #11018      +/-   ##
============================================
- Coverage     40.36%   40.35%   -0.01%     
- Complexity     4865     4871       +6     
============================================
  Files           943      950       +7     
  Lines         37061    37183     +122     
  Branches       4068     4078      +10     
============================================
+ Hits          14958    15004      +46     
- Misses        20600    20668      +68     
- Partials       1503     1511       +8     
Impacted Files Coverage Δ
...duler/plugin/task/zeppelin/ZeppelinParameters.java 70.00% <0.00%> (-11.25%) ⬇️
...heduler/plugin/task/jupyter/JupyterParameters.java 80.00% <0.00%> (-11.18%) ⬇️
...duler/api/service/impl/AccessTokenServiceImpl.java 78.65% <0.00%> (-10.38%) ⬇️
...scheduler/api/controller/DataSourceController.java 29.03% <0.00%> (-9.68%) ⬇️
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) ⬇️
...eduler/api/service/impl/DataSourceServiceImpl.java 28.47% <0.00%> (-1.02%) ⬇️
...permission/ResourcePermissionCheckServiceImpl.java 66.66% <0.00%> (-0.78%) ⬇️
...inscheduler/service/expand/CuringGlobalParams.java 33.33% <0.00%> (-0.39%) ⬇️
...nscheduler/service/process/ProcessServiceImpl.java 29.29% <0.00%> (ø)
...duler/api/security/impl/AbstractAuthenticator.java 100.00% <0.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@SbloodyS SbloodyS added the miss:docs missing documents in PR label Jul 18, 2022
@EricGao888 EricGao888 changed the title [improverment]:The install.sh file does not determine whether the use… [improverment]:The install.sh script does not check whether the user has sudo privileges Jul 18, 2022
@EricGao888
Copy link
Member

@ZYJBigData Hi, could you please rebase the code and push it again? It helps restart the CI. BTW, it would be better if you could keep the PR title complete : ) Thx~

@ZYJBigData ZYJBigData requested a review from EricGao888 as a code owner July 21, 2022 11:39
@zhuxt2015
Copy link
Contributor

I have a little doubt about that If the user does not have sudo permissions, should the worker be launched, because the task cannot be started without sudo permissions

@EricGao888
Copy link
Member

I have a little doubt about that If the user does not have sudo permissions, should the worker be launched, because the task cannot be started without sudo permissions

+1, instead of continuing, maybe the script should exit and output a message to instruct users to set up sudo permissions with a link to related doc or something. WDYT @ZYJBigData @chengshiwen @SbloodyS

@SbloodyS
Copy link
Member

I have a little doubt about that If the user does not have sudo permissions, should the worker be launched, because the task cannot be started without sudo permissions

+1, instead of continuing, maybe the script should exit and output a message to instruct users to set up sudo permissions with a link to related doc or something. WDYT @ZYJBigData @chengshiwen @SbloodyS

It seems that we have supported non sudo.

# use sudo or not, if set true, executing user is tenant user and deploy user needs sudo permissions; if set false, executing user is the deploy user and doesn't need sudo permissions
sudo.enable=true

zkRoot=${zkRoot:-"/dolphinscheduler"}

# whether the installation user has sudo permission
sudoEnable=${sudoEnable:-"true"}
Copy link
Member

Choose a reason for hiding this comment

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

Could you please update the docs about this? Thx

Copy link
Author

Choose a reason for hiding this comment

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

@EricGao888 OK soon, Thanks

@EricGao888
Copy link
Member

I have a little doubt about that If the user does not have sudo permissions, should the worker be launched, because the task cannot be started without sudo permissions

+1, instead of continuing, maybe the script should exit and output a message to instruct users to set up sudo permissions with a link to related doc or something. WDYT @ZYJBigData @chengshiwen @SbloodyS

It seems that we have supported non sudo.

# use sudo or not, if set true, executing user is tenant user and deploy user needs sudo permissions; if set false, executing user is the deploy user and doesn't need sudo permissions
sudo.enable=true

In that case, LGTM as long as related docs are updated.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

3.3% 3.3% Coverage
3.8% 3.8% Duplication

Comment on lines +33 to +39
if [ ${sudoEnable} = true ]; then
sudo mkdir -p $installPath
sudo chown -R $deployUser:$deployUser $installPath
else
mkdir -p $installPath
chown -R $deployUser:$deployUser $installPath
fi
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this? should we directly remove the sudo and require user have permission of installPath?

@github-actions
Copy link

github-actions bot commented Nov 6, 2023

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Nov 6, 2023
@github-actions
Copy link

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

@github-actions github-actions bot closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend document first time contributor First-time contributor miss:docs missing documents in PR Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants