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

[breaking] core update-index do not stop at the first download error #1866

Merged
merged 7 commits into from
Sep 9, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Sep 6, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?
    1. The core update-index do not stop anymore at the first error
    2. The UpdateIndex gRPC call has a new flag ignore_custom_package_indexes to allow skipping the update of custom package indexes on the first run.
  • What is the current behavior?
    If there is a problem downloading a package index the core update-index command will stop and not complete the update of the remaining custom indexes url.
  • What is the new behavior?
    The core update-index command will always complete the update of all indexes.

@cmaglie cmaglie force-pushed the do_not_stop_on_index_update_error branch from 6670d50 to 732bdc5 Compare September 6, 2022 12:23
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1866 (9fddd77) into master (71cab65) will increase coverage by 0.02%.
The diff coverage is 29.48%.

@@            Coverage Diff             @@
##           master    #1866      +/-   ##
==========================================
+ Coverage   36.62%   36.65%   +0.02%     
==========================================
  Files         231      231              
  Lines       19610    19655      +45     
==========================================
+ Hits         7182     7204      +22     
- Misses      11600    11622      +22     
- Partials      828      829       +1     
Flag Coverage Δ
unit 36.65% <29.48%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cli/core/search.go 0.00% <0.00%> (ø)
cli/core/update_index.go 0.00% <0.00%> (ø)
cli/update/update.go 0.00% <0.00%> (ø)
commands/daemon/daemon.go 0.00% <0.00%> (ø)
commands/instances.go 39.08% <18.42%> (-1.69%) ⬇️
cli/output/rpc_progress.go 68.75% <33.33%> (-5.80%) ⬇️
arduino/httpclient/httpclient.go 67.27% <100.00%> (ø)
cli/instance/instance.go 57.69% <100.00%> (+0.41%) ⬆️
internal/integrationtest/arduino-cli.go 83.73% <100.00%> (+0.54%) ⬆️
legacy/builder/hardware_loader.go 88.88% <0.00%> (-0.40%) ⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cmaglie cmaglie requested review from umbynos, per1234 and kittaakos and removed request for umbynos September 6, 2022 12:39
@cmaglie cmaglie self-assigned this Sep 6, 2022
@cmaglie cmaglie added type: enhancement Proposed improvement priority: high Resolution is a high priority topic: CLI Related to the command line interface topic: gRPC Related to the gRPC interface labels Sep 6, 2022
@cmaglie cmaglie requested a review from ubidefeo September 6, 2022 17:55
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Describe the problem

The value of the cc.arduino.cli.commands.v1.ArduinoCoreService.DownloadResult.successful field is set to true when a download fails.

To reproduce

Set up

$ arduino-cli version
arduino-cli.exe  Version: git-snapshot Commit: 732bdc54 Date: 2022-09-07T09:59:11Z

$ export ARDUINO_BOARD_MANAGER_ADDITIONAL_URLS=http://example.com/package_foo_index.json

$ arduino-cli daemon
Daemon is now listening on 127.0.0.1:50051

Demo

Use grpcurl to run the following commands in another terminal.

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.Create

{
  "instance": {
    "id": 1
  }
}

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.Init

{
  "error": {
    "code": 9,
    "message": "Loading index file: loading json index file C:\\Users\\per\\AppData\\Local\\Arduino15\\package_foo_index.json: open C:\\Users\\per\\AppData\\Local\\Arduino15\\package_foo_index.json: The system cannot find the file specified."
  }
}

$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/cli/commands/v1/commands.proto \
  -d '{"instance": {"id": 1}}' \
  127.0.0.1:50051 \
  cc.arduino.cli.commands.v1.ArduinoCoreService.UpdateIndex

{
  "downloadProgress": {
    "url": "https://downloads.arduino.cc/packages/package_index.tar.bz2",
    "file": "Downloading index: package_index.tar.bz2",
    "totalSize": "43541"
  }
}
{
  "downloadProgress": {
    "downloaded": "43541"
  }
}
{
  "downloadProgress": {
    "completed": true
  }
}
{
  "downloadResult": {
    "url": "https://downloads.arduino.cc/packages/package_index.tar.bz2",
    "successful": true
  }
}
{
  "downloadProgress": {
    "url": "http://example.com/package_foo_index.json",
    "file": "Downloading index: package_foo_index.json",
    "totalSize": "-1"
  }
}
{
  "downloadProgress": {
    "downloaded": "1256"
  }
}
{
  "downloadResult": {
    "url": "http://example.com/package_foo_index.json",
    "error": "Error downloading index 'http://example.com/package_foo_index.json': Server responded with: 404 Not Found"
  }
}
{
  "downloadResult": {
    "url": "http://example.com/package_foo_index.json",
    "successful": true
  }
}
ERROR:
  Code: Internal
  Message: Some indexes could not be updated.

🐛 The value of the cc.arduino.cli.commands.v1.ArduinoCoreService.DownloadResult.successful field was true for the failed download of http://example.com/package_foo_index.json.

Expected behavior

The value of the cc.arduino.cli.commands.v1.ArduinoCoreService.DownloadResult.successful field is set to false when the download fails.

@cmaglie
Copy link
Member Author

cmaglie commented Sep 7, 2022

🐛 The value of the cc.arduino.cli.commands.v1.ArduinoCoreService.DownloadResult.successful field was true for the failed download of http://example.com/package_foo_index.json.

I've pushed the fix for this problem, and the other one #1866 (comment)

@cmaglie cmaglie requested a review from per1234 September 7, 2022 14:35
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

The bugs reported in my previous review have been resolved and it is working as expected now.

Thanks Cristian!

@cmaglie cmaglie force-pushed the do_not_stop_on_index_update_error branch from c82517d to 9fddd77 Compare September 8, 2022 08:42
@cmaglie cmaglie merged commit c2bf718 into arduino:master Sep 9, 2022
@cmaglie cmaglie deleted the do_not_stop_on_index_update_error branch September 9, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high Resolution is a high priority topic: CLI Related to the command line interface topic: gRPC Related to the gRPC interface type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gRPC] It should be possible to run the core update-index equivalent with and without the 3rd party URLs
3 participants