download-plugins: raise error in case of errors downloading plugins

[perrinjerome]

What it does

Fixes: #7819.

If something went wrong while downloading plugins, the build should be
interrupted, instead of creating an environment where some plugins might
be missing.

How to test

One possible way is to add invalid entries in theiaPlugins from package.json, for example:

  "theiaPlugins": {
    "error-404": "https://open-vsx.org/404",
    "error-invalid-url": "not url"
  }

and use yarn download:plugins.

Without these changes, the command exits with success status code:

There are messages in red, yes, but the build report a success.

With the changes from this pull request, the command exit with error status code:

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

Technically this is a breaking change since this script is public and people might have relied on it always exiting with code 0.

Can you add a flag like --fail which will make it throw when set? Feel free to use a better name if you have one.

Also add an entry to the CHANGELOG to mention this new flag. Thanks :)

[perrinjerome]

Thank you, that's right, this would be a breaking change. I pushed more commits to introduce a --tolerate-download-failures flag and a CHANGELOG entry:

• [download:plugins] errors when downloading plugins now result in build failures, unless --tolerate-download-failures flag is passed. #8788

PS: I'm not sure I correctly understood the suggestion properly about what should be the default behaviour. Since the previous behaviour was reported as incorrect in #7819 , I was thinking that the default behaviour should become to fail on download error. Before I discovered the behaviour, I was assuming that unresolvable entries for plugins in package.json would cause the build to fail, just like the build fails when we there are unresolvable entries for npm packages in package.json. Also, if we keep the current behaviour of tolerating download failures by default and introduce an flag that users would have to pass to make download failures fatal, then this would not be enough to fix #7819 (we would have to do something such as updating documentation so that user pass the flag)

[vince-fugnitto]

I'm not sure I correctly understood the suggestion properly about what should be the default behaviour.

@perrinjerome the default should be to fail the build if there are errors when retrieving plugins (that is why it is marked as a breaking change. This aligns with the expectation from the issue.

[paul-marechal]

@perrinjerome I was asking to not change the default behavior and only throw when adding the flag, this way nothing breaks?

[paul-marechal]

After discussing with @vince-fugnitto it seems to make sense to throw by default, and have a --ignore-errors flag to prevent that. If anyone thinks the script should not throw please say so. This change might break some builds on error now.

[perrinjerome]

Thank you for your feedback. I have pushed a new commit to name the flag --ignore-errors / i

[vince-fugnitto]

@perrinjerome thank you for your changes 👍
Please squash your commits (into a single commit) so we can approve and merge :)

[perrinjerome]

Thank you, I squashed everything in one commit

[vince-fugnitto]

@perrinjerome it looks like there is currently a conflict with the changelog, please rebase, resolve conflicts and squash if necessary 👍

[perrinjerome]

@vince-fugnitto thanks ! I rebased and resolved conflict on changelog

[vince-fugnitto]

Thank you for the pull-request @perrinjerome!

I verified the following:

• the script successfully downloads plugins
• the script successfully reports errors when plugins cannot be fetched
• the -i or --ignore-errors flag correctly works in suppressing errors