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

Revert "Add root uid judgment before running" #182

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Revert "Add root uid judgment before running" #182

merged 1 commit into from
Nov 25, 2020

Conversation

sixg0000d
Copy link
Contributor

仔细想想不需要登陆 root 便能使用两个脚本的情况挺多的,比如一些路由器固件使用路由器管理页面管理员登录名作用户名且拥有读写这些位置的权限。
也许脚本需要一个权限检查,但 "$UID" -ne '0' 不一定是最好的选择。
其实早就想撤回这条提交了,但因为是自己提交的感觉有点尴尬

@nicholascw nicholascw merged commit 0941d67 into v2fly:develop Nov 25, 2020
@IceCodeNew
Copy link
Collaborator

IceCodeNew commented Nov 25, 2020

你这么取消 root 身份的检查以后,至少应该追加给其他命令补充 sudo 命令的提交。否则很可能会影响挺多人执行脚本的结果。
然后说到 sudo,在你说的这种情况里又往往可能不提供 sudo 程序,所以我觉得没法在这个项目上同时兼容两种不同的使用场景。
真的有需要折腾路由器的人,可以根据自己的需要 fork 本项目,cherry-pick 这个提交。

最后补充一个决定性的考虑:如果 ecc831a 顺利合并入 master 分支,那么这个脚本就非常有必要以 root 权限执行了。

@IceCodeNew
Copy link
Collaborator

我已经撤回了这次 PR。
这个 PR 没有加 sudo 就被合并了,我觉得非常不合理。
希望下次这种重大变更可以多等待其他成员的审核再进行合并。 @nicholascw

@nicholascw
Copy link
Contributor

emmm, 确实,没想到这么多。看起来 prompt warning 比 error 更合理?

check_if_running_as_root() {
  # If you want to run as another user, please modify $UID to be owned by this user
  if [[ "$UID" -ne '0' ]]; then
    echo "warning: Root permission may be required during the installation process."
    echo "To continue, press Enter. Otherwise, ^C to exit."
    read
  fi
}

@IceCodeNew
Copy link
Collaborator

emmm, 确实,没想到这么多。看起来 prompt warning 比 error 更合理?

check_if_running_as_root() {
  # If you want to run as another user, please modify $UID to be owned by this user
  if [[ "$UID" -ne '0' ]]; then
    echo "warning: Root permission may be required during the installation process."
    echo "To continue, press Enter. Otherwise, ^C to exit."
    read
  fi
}

我觉得这样改很好。

IceCodeNew added a commit that referenced this pull request Nov 25, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: IceCodeNew <32576256+IceCodeNew@users.noreply.github.com>
IceCodeNew added a commit to IceCodeNew/fhs-install-v2ray that referenced this pull request Nov 25, 2020
Co-authored-by: IceCodeNew <32576256+IceCodeNew@users.noreply.github.com>
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.

None yet

3 participants