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.

Sign up for GitHub

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

Jump to bottom

[BREAKING] gRPC / golang Configuration API refactoring #2565

Merged
merged 56 commits into from May 15, 2024
Merged

[BREAKING] gRPC / golang Configuration API refactoring #2565

merged 56 commits into from May 15, 2024

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Mar 13, 2024 •
edited

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?

This PR introduces a big change in the configuration API, the main goals for this PR are:

  • Making the gRPC API more prominent even in the internal of the golang code, in this case, after merging the PR, the command line interface implementation becomes 100% dependent on the gRPC API, (no more bypass)
  • The configuration API (hopefully) becomes more flexible than it was before, allowing a more granular control of the written fields
  • Makes real the 'third pillar' of the Arduino CLI (using the code by importing in another golang project).

What is the current behavior?

There should be very few changes in the CLI behavior, in theory, final users should not notice any change.

What is the new behavior?

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

Yes, the gRPC Settings* API changed a lot, adaption on the client is required.

Other information

@cmaglie cmaglie self-assigned this Mar 13, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 13, 2024 •
edited

Codecov Report

Attention: Patch coverage is 74.24299% with 689 lines in your changes are missing coverage. Please review.

Project coverage is 70.13%. Comparing base (03d8653) to head (cec1fdb).
Report is 4 commits behind head on master.

Files Patch % Lines
internal/go-configmap/types.go 46.52% 69 Missing and 8 partials ⚠️
commands/service_monitor.go 62.69% 53 Missing and 19 partials ⚠️
commands/service_settings.go 69.51% 39 Missing and 11 partials ⚠️
commands/cmderrors/cmderrors.go 14.89% 40 Missing ⚠️
commands/instances.go 72.91% 30 Missing and 9 partials ⚠️
commands/service_upload_burnbootloader.go 0.00% 35 Missing ⚠️
commands/service_upload.go 54.41% 29 Missing and 2 partials ⚠️
commands/service_compile.go 66.66% 24 Missing and 4 partials ⚠️
internal/go-configmap/cli.go 65.07% 16 Missing and 6 partials ⚠️
internal/cli/config/init.go 64.40% 12 Missing and 9 partials ⚠️
... and 50 more
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2565      +/-   ##
==========================================
+ Coverage   69.51%   70.13%   +0.61%     
==========================================
  Files         207      222      +15     
  Lines       20285    21124     +839     
==========================================
+ Hits        14102    14816     +714     
- Misses       5069     5132      +63     
- Partials     1114     1176      +62
Flag Coverage Δ
unit 70.13% <74.24%> (+0.61%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmaglie cmaglie force-pushed the config_refac_3 branch 3 times, most recently from 7d99eb4 to 99645aa Compare March 19, 2024 07:50
@cmaglie cmaglie mentioned this pull request Mar 25, 2024
Merged
6 tasks
@cmaglie cmaglie changed the title [DRAFT] refactoring experiments [DRAFT] gRPC / golang Configuration API refactoring experiments Mar 25, 2024
This was referenced Mar 25, 2024
Merged
Merged
Merged
@cmaglie cmaglie force-pushed the config_refac_3 branch 7 times, most recently from c92db6a to 593326d Compare April 22, 2024 12:42
@cmaglie cmaglie added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Apr 22, 2024
@cmaglie cmaglie changed the title [DRAFT] gRPC / golang Configuration API refactoring experiments [BREAKING] gRPC / golang Configuration API refactoring Apr 22, 2024
@cmaglie cmaglie force-pushed the config_refac_3 branch 2 times, most recently from e1583fc to 149c7e1 Compare April 23, 2024 10:41
@cmaglie cmaglie marked this pull request as ready for review April 23, 2024 13:24
@cmaglie cmaglie linked an issue Apr 23, 2024 that may be closed by this pull request
[cli] Expose configuration API to allow embedding in other Golang projects #2555
Closed
alessio-perugini
alessio-perugini approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@alessio-perugini alessio-perugini left a comment

Choose a reason for hiding this comment

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

LGTM

I'd say after the PR that unifies the usage of oneOf in our API, we can schedule a round of manual testing to catch any regression (if any) not seen by our integration tests.

umbynos
umbynos approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@umbynos umbynos left a comment •
edited

Choose a reason for hiding this comment

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

Overall LGTM, I did not find any major problem.
Should we update also this: https://github.com/arduino/arduino-cli/blob/master/docs/integration-options.md ?

internal/integrationtest/config/config_test.go Outdated Show resolved Hide resolved
@cmaglie
Copy link
Member Author

cmaglie commented May 15, 2024

Should we update also this: https://github.com/arduino/arduino-cli/blob/master/docs/integration-options.md?

Yes, BTW I'm doing it in another branch as part of a general revamp of the documentation.

@cmaglie cmaglie merged commit 8a85969 into arduino:master May 15, 2024
107 checks passed
@cmaglie cmaglie deleted the config_refac_3 branch May 15, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessio-perugini alessio-perugini alessio-perugini approved these changes

@umbynos umbynos umbynos approved these changes

@MatteoPologruto MatteoPologruto Awaiting requested review from MatteoPologruto

@Xayton Xayton Awaiting requested review from Xayton

Assignees

@cmaglie cmaglie

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

Successfully merging this pull request may close these issues.

[cli] Expose configuration API to allow embedding in other Golang projects
3 participants
@cmaglie @alessio-perugini @umbynos