Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Identify managed platforms not tracked by a package index #2174

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

Conversation

Copy link
Contributor

@alessio-perugini alessio-perugini commented May 9, 2023 •
edited
Loading

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?

We're introducing a warning message in the cli, when the user runs the upgrade of a platform that's no longer present in the additional-urls config.
We also modify a bit some proto messages.

What is the current behavior?

Right now when a platform is installed but the URL tracking the latest update for that platform gets removed from the config we're no longer to track the newest versions, and the user is not notified about it.

What is the new behavior?

When a platform is installed but the additional URL is no longer present In the config, we send a warning message in the stdout when the user tries to perform a core upgrade for that platform.

Protobuff changes:

  • message Platform:

    • added bool indexed = 13;
    • added bool missing_metadata = 14;

  • message PlatformUpgradeResponse:

    • added Platform platform = 3;

The rpc call PlatformUpgrade now also returns the Platform object which FE can use the indexed and missing_metadata to display some warnings. Like:

  • indexed = false -> warning
  • indexed = false AND missing_metdata = true -> warning

bonus:

  • missing_metadata = true -> we can decide to show a warning because the platform could be installed with the Arduino IDE 1.8

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

nope

Other information

I think we should discuss if we need a system to track down various warning messages.

per1234 reacted with thumbs up emoji
@alessio-perugini alessio-perugini linked an issue May 9, 2023 that may be closed by this pull request
3 tasks
@alessio-perugini alessio-perugini changed the title (削除) Identify managed platforms not tracked by a package index (削除ここまで) (追記) [draft] Identify managed platforms not tracked by a package index (追記ここまで) May 9, 2023
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from 2306d9e to 25ba5c8 Compare May 9, 2023 09:52
Copy link

codecov bot commented May 9, 2023 •
edited
Loading

Codecov Report

Patch coverage: 73.07% and project coverage change: +0.80 🎉

Comparison is base (855c238) 62.53% compared to head (07b6905) 63.34%.

Additional details and impacted files 
@@ Coverage Diff @@
## master #2174 +/- ##
==========================================
+ Coverage 62.53% 63.34% +0.80% 
==========================================
 Files 223 223 
 Lines 19488 19663 +175 
==========================================
+ Hits 12187 12455 +268 
+ Misses 6210 6123 -87 
+ Partials 1091 1085 -6
Flag Coverage Δ
unit 63.34% <73.07%> (+0.80%) ⬆️

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

Impacted Files Coverage Δ
commands/board/search.go 0.00% <0.00%> (ø)
commands/daemon/daemon.go 27.77% <33.33%> (+2.16%) ⬆️
arduino/cores/packagemanager/install_uninstall.go 55.40% <37.50%> (+0.98%) ⬆️
commands/instances.go 58.45% <57.89%> (+0.10%) ⬆️
internal/cli/core/upgrade.go 77.14% <70.00%> (+1.73%) ⬆️
commands/core/upgrade.go 80.64% <93.33%> (+30.64%) ⬆️
arduino/cores/cores.go 77.00% <100.00%> (+0.71%) ⬆️
arduino/cores/packageindex/index.go 90.33% <100.00%> (+0.29%) ⬆️
commands/board/listall.go 91.13% <100.00%> (+0.23%) ⬆️
commands/core.go 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

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

@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from 25ba5c8 to d33e77d Compare May 11, 2023 06:54
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch 3 times, most recently from 799e3a5 to c420bc0 Compare May 12, 2023 13:38
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from c420bc0 to 621052d Compare May 12, 2023 13:54
@alessio-perugini alessio-perugini changed the title (削除) [draft] Identify managed platforms not tracked by a package index (削除ここまで) (追記) Identify managed platforms not tracked by a package index (追記ここまで) May 12, 2023
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch 2 times, most recently from 9ecb877 to 605ac83 Compare May 12, 2023 14:46
@alessio-perugini alessio-perugini marked this pull request as ready for review May 12, 2023 15:09
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

Some nitpicks, the PR looks good overall.

alessio-perugini reacted with hooray emoji
if response == nil || response.Platform == nil {
return
}
if !response.Platform.Indexed || (response.Platform.MissingMetadata && !response.Platform.Indexed) {
Copy link
Member

Choose a reason for hiding this comment

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

!Indexed || (MissingMetada && !Indexed) is redundant you can just write: !Indexed.

BTW, the case of MissingMetadata && !Indexed is still valid and should print something along the line of:
Platform %s is missing installation metadata, it may not work properly or need re-installation.
but this should not be printed in the "core update" or "core install". The question is then, where to show it? maybe on compile or upload... Anyway, since all the required information is now available to the IDE via gRPC, I think we could merge this PR as-is and think about where to display the warning in a separate issue.

alessio-perugini reacted with thumbs up emoji
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from 0382c1f to c184f20 Compare May 22, 2023 14:03
@alessio-perugini alessio-perugini deleted the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch May 24, 2023 13:20
@per1234 per1234 added the topic: code Related to content of the project itself label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmaglie cmaglie cmaglie approved these changes

Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identify managed platforms not tracked by a package index

AltStyle によって変換されたページ (->オリジナル) /