Fix update.sh invocation with unknown variant #1161

LaurentGoderre · 2019-11-22T18:51:52Z

If update.sh was invoked with non-existing variant, it updated all
variants. Change in_variants_to_update to return true always if
second variant argument was not given, but not when given variant
argument does not match to available variants.

LaurentGoderre · 2019-11-22T19:00:58Z

This doesn't seem to work

Before this change:

./update.sh 8 alpine3.9,none

Updating version 8...
8/alpine3.9/Dockerfile is already up to date!
Done!

./update.sh 8 none

Updating version 8...
8/jessie/Dockerfile is already up to date!
8/jessie-slim/Dockerfile is already up to date!
8/stretch/Dockerfile is already up to date!
8/stretch-slim/Dockerfile is already up to date!
8/buster/Dockerfile is already up to date!
8/buster-slim/Dockerfile is already up to date!
8/onbuild/Dockerfile is already up to date!
8/alpine3.10/Dockerfile is already up to date!
8/alpine3.9/Dockerfile is already up to date!
Done!

After this change:

./update.sh 8 alpine3.9,none

Updating version 8...
8/alpine3.9/Dockerfile is already up to date!
Done!

./update.sh 8 none

./update.sh: line 96: update_variants[@]: unbound variable

If update.sh was invoked with non-existing variant, it updated all variants. Change in_variants_to_update to return true always if second variant argument was not given, but not when given variant argument does not match to available variants.

LaurentGoderre · 2019-11-22T19:05:40Z

Ok, problem fixed!

SimenB · 2019-12-03T22:11:22Z

update.sh

@@ -97,7 +97,7 @@ function in_variants_to_update() {
  local variant=$1

  if [ "${#update_variants[@]}" -eq 0 ]; then
-    echo 0
+    echo 1


Echo? Not exit?

Yeah because it's a function that needs to return a boolean. This causes the script to skip the unrecognized variant

PeterDaveHello

Maybe catch the error and give it a more clear prompt message?

LaurentGoderre · 2019-12-18T14:51:52Z

Can that be done in a separate PR? Right now it breaks but with the change it will ignore it.

PeterDaveHello · 2019-12-21T14:47:24Z

I prefer not to separate that to another PR, the change here did prevent a strange situation but also added one, line 96: update_variants[@]: unbound variable is not really understandable, and kind of exception, which IMO shouldn't be made.

LaurentGoderre · 2019-12-23T13:47:24Z

How do you get that error?

PeterDaveHello · 2020-01-02T09:03:30Z

I mean update_variants[@]: unbound variable

LaurentGoderre · 2020-01-06T14:40:21Z

I used to get that error but this PR fixes this.

PeterDaveHello · 2020-01-07T05:20:37Z

This doesn't seem to work

Before this change:

./update.sh 8 alpine3.9,none

Updating version 8...
8/alpine3.9/Dockerfile is already up to date!
Done!

./update.sh 8 none

Updating version 8...
8/jessie/Dockerfile is already up to date!
8/jessie-slim/Dockerfile is already up to date!
8/stretch/Dockerfile is already up to date!
8/stretch-slim/Dockerfile is already up to date!
8/buster/Dockerfile is already up to date!
8/buster-slim/Dockerfile is already up to date!
8/onbuild/Dockerfile is already up to date!
8/alpine3.10/Dockerfile is already up to date!
8/alpine3.9/Dockerfile is already up to date!
Done!

After this change:

./update.sh 8 alpine3.9,none

Updating version 8...
8/alpine3.9/Dockerfile is already up to date!
Done!

./update.sh 8 none

./update.sh: line 96: update_variants[@]: unbound variable

I thought update_variants[@]: unbound variable is after the change?

LaurentGoderre · 2020-01-07T16:59:24Z

Aaaah,that comment was before I fixed it.

PeterDaveHello · 2020-01-09T04:28:04Z

Oh, I misunderstood, sorry!

nschonni · 2020-01-10T01:27:44Z

Tried to run the update.sh for 10.18.1, but it wouldn't work till I reverted this locally
Edit: Running ./update.sh 10 on OSX with zsh (the new default shell)

LaurentGoderre · 2020-01-10T14:22:16Z

@nschonni do you have more details?

SimenB · 2020-01-10T14:28:38Z

Running ./update.sh 10 does nothing (you can revert the last commit so it's on 10.18.0 again). Running just ./update.sh does nothing either. Unsure why this change makes a difference, though...

LaurentGoderre · 2020-01-10T14:34:05Z

Really?

LaurentGoderre · 2020-01-10T14:35:51Z

I don't think it's necessarily this one that broke it but the one before that.

SimenB · 2020-01-10T14:38:13Z

No, just flipping the 1 back to 0 fixes it. You can test on master.

$ ./update.sh
Updating version 10...
Updating version 12...
Updating version 13...
Updating version chakracore/10...
chakracore/10/Dockerfile is already up to date!
Done!
$ git revert --no-edit 6c3fa2f322cbff89885a849a7dfe69972ce97a30
Auto-merging update.sh
[master e961171] Revert "Fix update.sh invocation with unknown variant"
 Date: Fri Jan 10 15:37:30 2020 +0100
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git diff origin/master
diff --git c/update.sh w/update.sh
index 25d61dc..87f1c86 100755
--- c/update.sh
+++ w/update.sh
@@ -97,7 +97,7 @@ function in_variants_to_update() {
   local variant=$1
 
   if [ "${#update_variants[@]}" -eq 0 ]; then
-    echo 1
+    echo 0
     return
   fi

$ ./update.sh
Updating version 10...
Updating version 12...
Updating version 13...
10/jessie/Dockerfile is already up to date!
10/jessie-slim/Dockerfile is already up to date!
10/stretch/Dockerfile is already up to date!
10/buster/Dockerfile is already up to date!
10/buster-slim/Dockerfile is already up to date!
12/stretch/Dockerfile is already up to date!
12/buster/Dockerfile is already up to date!
10/stretch-slim/Dockerfile is already up to date!
12/buster-slim/Dockerfile is already up to date!
Updating version chakracore/10...
12/stretch-slim/Dockerfile is already up to date!
13/buster/Dockerfile is already up to date!
13/stretch/Dockerfile is already up to date!
13/stretch-slim/Dockerfile is already up to date!
13/buster-slim/Dockerfile is already up to date!
10/alpine3.10/Dockerfile is already up to date!
10/alpine3.11/Dockerfile is already up to date!
10/alpine3.9/Dockerfile is already up to date!
chakracore/10/Dockerfile is already up to date!
12/alpine3.9/Dockerfile is already up to date!
12/alpine3.11/Dockerfile is already up to date!
12/alpine3.10/Dockerfile is already up to date!
13/alpine3.10/Dockerfile is already up to date!
13/alpine3.11/Dockerfile is already up to date!
Done!

LaurentGoderre · 2020-01-10T14:43:23Z

You're right. #1192

LaurentGoderre mentioned this pull request Nov 22, 2019

Fix update.sh invocation with slim #1070

Closed

LaurentGoderre force-pushed the improve-updatesh branch from 5bab97c to 6c3fa2f Compare November 22, 2019 19:04

LaurentGoderre requested review from PeterDaveHello and SimenB November 26, 2019 19:11

SimenB reviewed Dec 3, 2019

View reviewed changes

SimenB approved these changes Dec 18, 2019

View reviewed changes

PeterDaveHello reviewed Dec 18, 2019

View reviewed changes

PeterDaveHello approved these changes Jan 9, 2020

View reviewed changes

PeterDaveHello merged commit e088f69 into nodejs:master Jan 9, 2020

LaurentGoderre deleted the improve-updatesh branch January 9, 2020 14:56

nschonni mentioned this pull request Jan 10, 2020

Documentation request: how to update files for a new node version #1190

Closed

This was referenced Jan 10, 2020

Fixed update script when no variant is provided #1191

Closed

Revert "Fix update.sh invocation with unknown variant" #1192

Merged

Fix update.sh invocation with unknown variant #1161

Fix update.sh invocation with unknown variant #1161

Uh oh!

Conversation

LaurentGoderre commented Nov 22, 2019

Uh oh!

LaurentGoderre commented Nov 22, 2019

Uh oh!

LaurentGoderre commented Nov 22, 2019

Uh oh!

SimenB Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

LaurentGoderre Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello left a comment

Choose a reason for hiding this comment

Uh oh!

LaurentGoderre commented Dec 18, 2019

Uh oh!

PeterDaveHello commented Dec 21, 2019

Uh oh!

LaurentGoderre commented Dec 23, 2019

Uh oh!

PeterDaveHello commented Jan 2, 2020

Uh oh!

LaurentGoderre commented Jan 6, 2020

Uh oh!

PeterDaveHello commented Jan 7, 2020

Uh oh!

LaurentGoderre commented Jan 7, 2020

Uh oh!

PeterDaveHello commented Jan 9, 2020

Uh oh!

nschonni commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaurentGoderre commented Jan 10, 2020

Uh oh!

SimenB commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaurentGoderre commented Jan 10, 2020

Uh oh!

LaurentGoderre commented Jan 10, 2020

Uh oh!

SimenB commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaurentGoderre commented Jan 10, 2020

Uh oh!

Uh oh!

nschonni commented Jan 10, 2020 •

edited

Loading

SimenB commented Jan 10, 2020 •

edited

Loading

SimenB commented Jan 10, 2020 •

edited

Loading