[BREAKING] feat: added details to all init errors

[Bikappa]

Please check if the PR fulfills these requirements

See how to contribute

• 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)
• configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Iterates on #2076 adding details to more errors in the instance initialization flow

What is the current behavior?

One case of index loading error and some other error scenarios do not have details

{
  "error": {
    "code": 9,
    "message": "Loading index file: reading library_index.json: open /Users/a.kitta/Library/Arduino15/library_index.json: no such file or directory"
  }
}

Platform loading errors are not reported as initialization errors

{
  "error": {
    "code": 9,
    "message": "Error loading hardware platform: discovery builtin:mdns-discovery not found",
    "details": [
      {"@type":"type.googleapis.com/cc.arduino.cli.commands.v1.PlatformLoadingError"}
    ]
  }
}

What is the new behavior?

All init errors have a the corresponding details

{
  "error": {
    "code": 9,
    "message": "Loading index file: reading library_index.json: open /Users/bikappa/Library/Arduino15/library_index.json: no such file or directory"
   "details": [
         {"@type":"type.googleapis.com/cc.arduino.cli.commands.v1.FailedInstanceInitError", "message":"Loading index file: reading library_index.json: open /Users/bikappa/Library/Arduino15/library_index.json: no such file or directory","reason":"FAILED_INSTANCE_INIT_REASON_INDEX_LOAD_ERROR"
    ]
  }
}

Platform errors are wrapped into init errors and given a dedicated reason

{
  "error": {
    "code": 9,
    "message": "Error loading hardware platform: discovery builtin:serial-discovery not found",
    "details": [
      {"@type":"type.googleapis.com/cc.arduino.cli.commands.v1.FailedInstanceInitError", "message": "Error loading hardware platform: discovery builtin:serial-discovery not found", "reason": "FAILED_INSTANCE_INIT_REASON_PLATFORM_LOAD_ERROR"}
    ]
  }
}

Does this PR introduce a breaking change, and is titled accordingly?

Yes,PlatformLoadingError disappear as gprc type. Initialization errors related to platform loads now are have a detail of type FailedInstanceInitError as in the example above.

Other information

Relates to #1762

[codecov]

Codecov Report

Patch coverage: 20.00% and project coverage change: +0.03 🎉

Comparison is base (2900744) 35.01% compared to head (ef5cbe2) 35.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2097      +/-   ##
==========================================
+ Coverage   35.01%   35.05%   +0.03%     
==========================================
  Files         232      232              
  Lines       20560    20560              
==========================================
+ Hits         7200     7208       +8     
+ Misses      12513    12505       -8     
  Partials      847      847              

Flag	Coverage Δ
unit	35.05% <20.00%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
commands/instances.go	38.84% <10.00%> (+0.19%)	⬆️
arduino/errors.go	7.74% <100.00%> (+2.78%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kittaakos]

@Bikappa, could you please rebase the PR from the master? On IDE2's side, I need this change but also need #2102 to have zero compiler errors. Thank you!

Update

Never mind, I did it on https://github.com/kittaakos/arduino-cli/tree/feat/more-error-details

[kittaakos]

Thank you!

Two things still need to be fixed. When the library_index.json is missing, IDE2 expects reason to be FAILED_INSTANCE_INIT_REASON_LIBRARY_LOAD_ERROR (4), but it isn't. All errors when deleting the directories.data location and initializing the gRPC client.

[
    {
        "reason": 2,
        "message": "Loading index file: loading json index file /Users/a.kitta/Library/Arduino15/package_index.json: open /Users/a.kitta/Library/Arduino15/package_index.json: no such file or directory"
    },
    {
        "reason": 2,
        "message": "Loading index file: loading json index file /Users/a.kitta/Library/Arduino15/package_esp8266com_index.json: open /Users/a.kitta/Library/Arduino15/package_esp8266com_index.json: no such file or directory"
    },
    {
        "reason": 2,
        "message": "Loading index file: loading json index file /Users/a.kitta/Library/Arduino15/package_esp32_index.json: open /Users/a.kitta/Library/Arduino15/package_esp32_index.json: no such file or directory"
    },
    {
        "reason": 2,
        "message": "Loading index file: loading json index file /Users/a.kitta/Library/Arduino15/package_teensy_index.json: open /Users/a.kitta/Library/Arduino15/package_teensy_index.json: no such file or directory"
    },
    {
        "reason": 5,
        "message": "discovery builtin:serial-discovery not found"
    },
    {
        "reason": 5,
        "message": "discovery builtin:mdns-discovery not found"
    },
    {
        "reason": 2,
        "message": "Loading index file: reading library_index.json: open /Users/a.kitta/Library/Arduino15/library_index.json: no such file or directory"
    }
]

From the errors 👆, IDE2 does not know if primary package index loading has failed or if it's a 3rd party platform.

[Bikappa]

@kittaakos the error code has been updated to be the one you expect.

In regards to the kind of origin (builtin or 3rd) party, I tried looking into the code, but it turns out to be easier said than done. The current logic has to be stretched out quite a bit in order to collect the info about the origin and bring it to the output. I also don't know if we really want to maintain that.

I propose to merge this as-is now and start thinking about a common error source design that provides structured metadata for non-human clients. For example:

{
type: "Error",
message: "An error occurred while installing library \"foo\".",
...,
cause: {
  entities: [{
    type: "Library",
    vendor: "Arduino"
    name: "Foo",
    version: "x.y.z",
    managed: true // --> comes out-of-the-box or not
  }],
  action: "install",
  offence: "missing" | "conflict" | ...,
  remediations: [{
  ... 
 }]
}

It would then be up to the client to define their own heuristic on the software status and wether or not it is operable for their purposes.

[kittaakos]

the error code has been updated to be the one you expect.

Thank you, @Bikappa

I also don't know if we really want to maintain that.

IDE2 needs to decide somehow.

propose to merge this as-is now

Sure, I will discard my review, and you can proceed with the merge. On the other hand, IDE2 would be the only client of this API but cannot use it, so merging this PR does not change anything.

common error source design that provides structured metadata for non-human clients. For example:

IDE2 has been waiting for this since day one. I am sure there could be a more sophisticated way of propagating application-specific errors between IDE2 and CLI, but surely adding another enum type FAILED_INSTANCE_INIT_REASON_INDEX_LOAD_ERROR -> FAILED_INSTANCE_INIT_REASON_PRIMARY_INDEX_LOAD_ERROR seems a lot easier to indicate what has gone wrong.

See #2097 (comment)

[Bikappa]

FAILED_INSTANCE_INIT_REASON_INDEX_LOAD_ERROR -> FAILED_INSTANCE_INIT_REASON_PRIMARY_INDEX_LOAD_ERROR seems a lot easier to indicate what has gone wrong.

Surely it looks an easy design, but, maybe not for this specific one, several errors come to the initialisation logic similarly to how clients see them: as a black-box "error" thing that has a message. And these are just forwarded.
I'd like to not proceed in an patch-and-fix strategy, but have a coherent error bubble up strategy for all of them, at least in principle, cause we all know it won't be perfect :)

[cmaglie]

From the errors point_up_2, IDE2 does not know if primary package index loading has failed or if it's a 3rd party platform.

I think this will be solved when we fix #1529: it basically requires that the first Init call will trigger an index-update if the main index is missing, this would offload the IDE from the duty of understanding the reason of the failure and triggering an update on his own.

[cmaglie]

#1529 should have made the Init process much simpler and removed the need for most of the error code exposed here.