add auto-update for the d-keyring #457

AntonOks · 2020-06-13T13:26:53Z

@wilzbach based on the dlang/dlang.org#2817 discussion

dlang-bot · 2020-06-13T13:26:55Z

Thanks for your pull request and interest in making D better, @AntonOks! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

…ttps://www.shellcheck.net

AntonOks · 2020-06-20T20:46:59Z

@wilzbach @MartinNowak Hi there. Seems like all checks are ok finally. Is there anything else needed or desired or are you also ok with this?

thanks and regards

AntonOks · 2020-07-12T19:41:59Z

3 weeks passed by... any feedback from anyone would be appreciated...

This reverts commit 41a1da1.

wilzbach · 2020-09-03T20:37:07Z

script/install.sh

@@ -5,6 +5,8 @@
 # Authors: Martin Nowak
 # Documentation: https://dlang.org/install.html

+# set -x


Shouldn't have been committed

forgot this after "debugging", but then again, it is commented out and shouldn't harm, that's why I finally decided to not delete it again. anyhow, feel free to remove it...

wilzbach · 2020-09-03T20:37:23Z

script/install.sh

@@ -80,7 +82,7 @@ retry() {
    for i in {0..4}; do
        if "$@"; then
            break
-        elif [ $i -lt 4 ]; then
+        elif [ "$i" -lt 4 ]; then


i is a number, not a string.

correct.
if I'm nor wrong, I had it like you suggest it now initially but "shellcheck" complained so I made it as it is... also here, feel free to change it to whatever fit's your standards / requirements...

wilzbach · 2020-09-03T20:43:16Z

script/install.sh

+    # check if tools where installed already and update d-keyring.gpg if necessary
+    local tmp
+    tmp=$(mkdtemp)
+    if [ -f "$ROOT/d-keyring.gpg" ] && [ "$(command curl https://dlang.org/d-keyring.gpg -z "$ROOT/d-keyring.gpg" -o "$tmp/d-keyring.gpg" -s -L -w "%{http_code}")" == "200" ] ; then


This is the real problem:

This probably needs retry logic

Probably needs the fallback mirrors

Should verify the authenticity

Shouldn't re-download the file(s) if they haven't changed

Needs to update install.sh as well (the script changes a lot more often than the keyring)

Needs to provide the user an option to opt-out

Should probably use "Last-Modified", "ETag" or similar to avoid unnecessary downloads

Auto-updates need to be documented in help page, documentation and changelog (there are serious security concerns with it)

Should have a test

this all together is far beyond my bash foo and also the time I can / want spend on this topic, sorry...
nevertheless, some comments from here...

=> make's sense but I couldn't find a (smart & easy) way how to check if the file was changed without downloading it. actually I did read plenty of "curl way" post / blogs, i.e. https://blog.yjl.im/2012/03/downloading-only-when-modified-using.html and https://www.thegeekstuff.com/2012/04/curl-examples/ but at the end it seems like curl ALLWAYS downloads the file to be able to check for a change... and as it's downloaded anyhow, I decided to implement it as it is

=> well, if the script itself should also be updated, neither this change nor the "update" option of the script makes any sense... because in other words you ask to make the already existing"update" option an always applied defauld (from my point of view, I wouldn't like this kind of change... I want to know and control when and why a tool gets updated)

=> see my comments on topic nr4

This reverts commit 41a1da1.

wilzbach · 2020-09-03T20:47:52Z

Hi, I'm really sorry for not responding (things are rather busy), but this shouldn't have been merged without review. I triggered a revert, s.t. this change doesn't get accidentally deployed before we are ready to announce it. I am aware that this is a very frustrating experience and I will try to be a lot more active next time and if you understandably decide to not resubmit, I will try to find time to champion it. Sorry.

AntonOks · 2020-09-04T20:07:04Z

Hi, I'm really sorry for not responding (things are rather busy), but this shouldn't have been merged without review. I triggered a revert, s.t. this change doesn't get accidentally deployed before we are ready to announce it. I am aware that this is a very frustrating experience and I will try to be a lot more active next time and if you understandably decide to not resubmit, I will try to find time to champion it. Sorry.

Hey Seb, no prob and no need for apologias.
As people wrote so many time in the forum, Dlang is only community driven anyhow... and I know you and a little group of "key contributer's" improved Dlang and it's eco system A LOT... so big thanks for all your efforts!

andre2007 · 2020-10-05T14:59:56Z

@AntonOks
What do you think about following interaction flow:
In case of an key ring error there is a prompt for updating the file.
In addition there is also a command line argument to force the key ring update (false by default).

add auto-update for the d-keyring

921fed9

AntonOks added 2 commits June 20, 2020 21:49

correct some Travis CI / shellcheck errors

e6cc835

correct some Travis CI / shellcheck errors.. check is now OK due to h…

afe6c9f

…ttps://www.shellcheck.net

thewilsonator approved these changes Sep 2, 2020

View reviewed changes

thewilsonator added the auto-merge-squash Like auto-merge, but squash all the commits into one label Sep 2, 2020

dlang-bot merged commit 41a1da1 into dlang:master Sep 2, 2020

AntonOks deleted the add_auto_update branch September 2, 2020 11:30

wilzbach added a commit that referenced this pull request Sep 3, 2020

Revert "add auto-update for the d-keyring (#457)"

fbcd475

This reverts commit 41a1da1.

wilzbach reviewed Sep 3, 2020

View reviewed changes

dlang-bot pushed a commit that referenced this pull request Sep 3, 2020

Revert "add auto-update for the d-keyring (#457)"

7b0a273

This reverts commit 41a1da1.

wilzbach mentioned this pull request Sep 8, 2020

Always update the install script #302

Open

AntonOks mentioned this pull request Nov 22, 2020

scripts: prevent globbing and word splitting and other improvements #473

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add auto-update for the d-keyring #457

add auto-update for the d-keyring #457

AntonOks commented Jun 13, 2020

dlang-bot commented Jun 13, 2020

AntonOks commented Jun 20, 2020

AntonOks commented Jul 12, 2020

wilzbach Sep 3, 2020

AntonOks Sep 4, 2020

wilzbach Sep 3, 2020

AntonOks Sep 4, 2020

wilzbach Sep 3, 2020 •

edited

Loading

AntonOks Sep 4, 2020

wilzbach commented Sep 3, 2020

AntonOks commented Sep 4, 2020

andre2007 commented Oct 5, 2020

add auto-update for the d-keyring #457

add auto-update for the d-keyring #457

Conversation

AntonOks commented Jun 13, 2020

dlang-bot commented Jun 13, 2020

Bugzilla references

AntonOks commented Jun 20, 2020

AntonOks commented Jul 12, 2020

wilzbach Sep 3, 2020

Choose a reason for hiding this comment

AntonOks Sep 4, 2020

Choose a reason for hiding this comment

wilzbach Sep 3, 2020

Choose a reason for hiding this comment

AntonOks Sep 4, 2020

Choose a reason for hiding this comment

wilzbach Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

AntonOks Sep 4, 2020

Choose a reason for hiding this comment

wilzbach commented Sep 3, 2020

AntonOks commented Sep 4, 2020

andre2007 commented Oct 5, 2020

wilzbach Sep 3, 2020 •

edited

Loading