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

Add retrying to installer #7348

Merged
merged 7 commits into from Jul 7, 2016
Merged

Add retrying to installer #7348

merged 7 commits into from Jul 7, 2016

Conversation

tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Jul 4, 2016

For #7266. This is code copied from install.meteor.com. The script seems to work, as far as I can test it.


# Only show progress bar animations if we have a tty
# (Prevents tons of console junk when installing within a pipe)
if [[ -t 1 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the additions look POSIX-compatible, but because /bin/sh isn't always bash (GNU), but sometimes dash such as on Ubuntu, the [[ will break things (and currently is, see #7353).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoa, thanks @abernix, what would we do without you :)

Just to fill you in: this is the script installed at /usr/local/bin/meteor, which is run with bash I guess, thanks to the shebang.

The script at install.meteor.com is similar but part of a private repo (the package server). I foolishly tried to unify them :/

Copy link
Contributor

@abernix abernix Jul 4, 2016

Choose a reason for hiding this comment

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

Yeah, I just sussed out this loader a couple weeks ago, but I incorrectly assumed the shebang was the same as install.meteor.com. So I guess this is fine but generally it's best to keep POSIX-compliant – though I find it highly frustrating exactly because of issues like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, on the install.meteor.com script, I just noticed that you completely removed the "IF TTY USE progress-bar" conditional. You should be fine to continue using that so long as you only use single brackets instead of double-brackets:

if [ -t 1 ]; then

Which I can confirm works in Debian properly when running the command without an interactive terminal (via SSH). I didn't test with anything else (Docker, for example), but it should be the same as there is no STDIN descriptor.

(Also, I realize this is the wrong issue to discuss the install.meteor.com script on, however I don't have a better PR for it since it's not in the repo. 😉)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just reverted it quickly. I'll give that a try (at a more considered pace this time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deployed now. Let me know if you hear of any problems.

@tmeasday
Copy link
Contributor Author

tmeasday commented Jul 6, 2016

@benjamn I'll ship this change to the installer first and ping back here in a day or two when we know it has no problems.

else
curl -s --fail "$TARBALL_URL" | tar -xzf - -C "$INSTALL_TMPDIR"
fi
TARBALL_FILE="$(dirname "$METEOR_WAREHOUSE_DIR")/.meteor-tarball-tmp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this file in a way that makes it unique for a given $TARBALL_URL, perhaps by hashing the URL and appending the hash? I can imagine someone failing to download one tarball and trying a different version, only to end up appending to the old temporary file, resulting in an invalid download.

@benjamn benjamn force-pushed the 7266-add-retrying-to-installer branch from 81a49d3 to 876e122 Compare July 6, 2016 18:32
Hammering the server 98 times after a 404 seems excessive.
else
curl -s --fail "$TARBALL_URL" -C - -o "$TARBALL_FILE"
curl --silent --fail --continue-at - \
"$TARBALL_URL" --output "$TARBALL_FILE"
Copy link
Contributor Author

@tmeasday tmeasday Jul 6, 2016

Choose a reason for hiding this comment

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

Should we just switch the "verbosity" part out with something like

VERBOSITY="--silent";
if [ -t 1 ]; then
  VERBOSITY="--progressbar"
fi

# later

curl $VERBOSITY --fail...

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems nice and DRY :)

@tmeasday
Copy link
Contributor Author

tmeasday commented Jul 7, 2016

I think this is ready to go. I've just deployed the equivalent code to install.meteor.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