Skip to content

Commit

Permalink
remove silent flag for cURL calls in install-dat-release.sh
Browse files Browse the repository at this point in the history
  • Loading branch information
Nicholas Wang authored Aug 17, 2020
1 parent 4554467 commit 82f1734
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions install-dat-release.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ check_if_running_as_root() {
}

download_geoip() {
if ! curl -s -L -H 'Cache-Control: no-cache' -o "${V2RAY}geoip.dat.new" "$DOWNLOAD_LINK_GEOIP"; then
if ! curl -L -H 'Cache-Control: no-cache' -o "${V2RAY}geoip.dat.new" "$DOWNLOAD_LINK_GEOIP"; then
echo 'error: Download failed! Please check your network or try again.'
exit 1
fi
if ! curl -s -L -H 'Cache-Control: no-cache' -o "${V2RAY}geoip.dat.sha256sum.new" "$DOWNLOAD_LINK_GEOIP.sha256sum"; then
if ! curl -L -H 'Cache-Control: no-cache' -o "${V2RAY}geoip.dat.sha256sum.new" "$DOWNLOAD_LINK_GEOIP.sha256sum"; then
echo 'error: Download failed! Please check your network or try again.'
exit 1
fi
Expand All @@ -44,11 +44,11 @@ download_geoip() {
}

download_geosite() {
if ! curl -s -L -H 'Cache-Control: no-cache' -o "${V2RAY}geosite.dat.new" "$DOWNLOAD_LINK_GEOSITE"; then
if ! curl -L -H 'Cache-Control: no-cache' -o "${V2RAY}geosite.dat.new" "$DOWNLOAD_LINK_GEOSITE"; then
echo 'error: Download failed! Please check your network or try again.'
exit 1
fi
if ! curl -s -L -H 'Cache-Control: no-cache' -o "${V2RAY}geosite.dat.sha256sum.new" "$DOWNLOAD_LINK_GEOSITE.sha256sum"; then
if ! curl -L -H 'Cache-Control: no-cache' -o "${V2RAY}geosite.dat.sha256sum.new" "$DOWNLOAD_LINK_GEOSITE.sha256sum"; then
echo 'error: Download failed! Please check your network or try again.'
exit 1
fi
Expand Down

10 comments on commit 82f1734

@IceCodeNew
Copy link
Collaborator

Choose a reason for hiding this comment

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

关于 Wiki/README 的修改可以根据情况判断直接 commit 到 master 分支上，但是对于脚本的修改希望还是先推到 develop 分支上，经过其他成员审阅后合入 master 分支，谢谢。

@nicholascw
Copy link
Contributor

Choose a reason for hiding this comment

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

关于 Wiki/README 的修改可以根据情况判断直接 commit 到 master 分支上，但是对于脚本的修改希望还是先推到 develop 分支上，经过其他成员审阅后合入 master 分支，谢谢。

  • is a minor change
  • notified other members in advance
  • so-called reviewing process is broken anyway (see 46ccbf0 c73193c)

@IceCodeNew
Copy link
Collaborator

Choose a reason for hiding this comment

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

关于 Wiki/README 的修改可以根据情况判断直接 commit 到 master 分支上，但是对于脚本的修改希望还是先推到 develop 分支上，经过其他成员审阅后合入 master 分支，谢谢。

  • is a minor change
  • notified other members in advance
  • so-called reviewing process is broken anyway (see 46ccbf0 c73193c)

第一条我不同意，我在隔壁 domain-community 也有一样的理由这么直接 commit 了一次，还是被建议先提 PR
不过你说这次你事先通知了其他成员的话那还好
我不觉得人工审阅有一次缺漏就足以让人放弃这条路。

@IceCodeNew
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW 你说这是一个 minor change，但其实你已经改变了脚本的行为
旧的 install-dat-release.sh 脚本记得完全没有 stdout 输出，可以直接直接放到 crontab 里定时执行
但是你这么改变过行为过后，想直接在 crontab 里执行这个脚本，就需要先配置发信服务，或者手动把 stdout 丢到 /dev/null 里

@IceCodeNew
Copy link
Collaborator

Choose a reason for hiding this comment

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

// 题外话
@dctxmei 我打算如下调整一下 develop 分支，不会有任何副作用，看你可以接受么？

git checkout develop
git reset --hard master
git push

@kslr
Copy link
Contributor

@kslr kslr commented on 82f1734 Aug 17, 2020

Choose a reason for hiding this comment

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

目前普遍成员位于 Contributors 团队，为了方便考虑拥有写权限，但实际上可能需要其他人审阅。
Developers 团队可以获得默认维护者的权限。

@nicholascw
Copy link
Contributor

@nicholascw nicholascw commented on 82f1734 Aug 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

我不觉得人工审阅有一次缺漏就足以让人放弃这条路。

  • Large portion of this repository is actually not cross-reviewed reflected from the history, and, I've got @vcptr @studentmain @EpLiar @kslr informed and @kslr agreed on the commits. However I agree to the enforcement of the reviewing process in future commit. Let's stop the debate.

Speaking of the reason removing the silent flags, there's a sign showing there's insufficient debug information in the case cURL fails, and such a shell script is ought to be spitting out more information rather than keep it totally silent, especially when calling external binaries and the script itself cannot gather sufficient debug information in the case it fails. (see https://t.me/v2fly_chat/149571)

旧的 install-dat-release.sh 脚本记得完全没有 stdout 输出，可以直接直接放到 crontab 里定时执行

In my opinion, using stdout is of course expected unless it was explicitly closed in previous versions, and the error string could also using stdout as well.

BTW in almost all cases, expecting specific output, or no output, from an opened file descriptor is a dangerous, irresponsive, and inappropriate way of handling it, especially if it could fail your configuration.

@IceCodeNew
Copy link
Collaborator

Choose a reason for hiding this comment

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

Such a shell script is ought to be spitting out more information rather than keep it totally silent

我并没有反对你的修改，其实我也觉得删掉 -s 挺好的。
但是我就想以这个为例和你讲一下，你认为是 minor change 的东西，可能会在你未曾想到的方面对用户造成影响。
所以最好是不要凭自己一个人的判断决定要不要通过 PR 来提交这个更改——实际上这个改动并不很着急，那有规矩可循的时候又何必另辟蹊径呢？

@nicholascw
Copy link
Contributor

@nicholascw nicholascw commented on 82f1734 Aug 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

改动并不很着急

You should realized that using silent flags is making the downloading URL / IP resolved to totally invisible to users. Therefore the risk of supplychain attack and MITM attack could be significantly higher. You may also noticed that a Cloudflare worker service (raw.githubusercontent.workers.dev) deployed by undisclosed party for downloading systemd service files was introduced in install-release.sh, which is not even acknowledged by most of our organization developers and contributors after lied there for 4 months, also it seems most users would discover this as well if cURL remains silent, and further more it seems it was causing problems.

你未曾想到的方面对用户造成影响

Not for those who lie on active railroads for photos and think no train would use it.

@IceCodeNew
Copy link
Collaborator

Choose a reason for hiding this comment

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

// 题外话
@dctxmei 我打算如下调整一下 develop 分支，不会有任何副作用，看你可以接受么？

git checkout develop
git reset --hard master
git push

已经超过 24 小时过去了，因为 develop 分支没有跟上 master 分支，已经造成最近的一次合并出现冲突。
所以我现在要自作主张，调整 develop 分支和 master 分支同步了。

Please sign in to comment.