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] Fix board attach CLI command, and remove gRPC API #1930

Merged
merged 23 commits into from Nov 21, 2022

Conversation

silvanocerza
Copy link
Contributor

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)

What kind of change does this PR introduce?

Remove an unused neglected broken feature.

What is the current behavior?

board attach CLI command and BoardAttach gRPC interface command are both broken and neglected. Their behaviour is unclear and buggy.

What is the new behavior?

Remove board attach CLI command and BoardAttach gRPC interface command and all their usages from the project.

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

Yes and yes.

Other information

Closes #1925.

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.

  • There is this test to remove:
    board_test.go:452: 
        	Error Trace:	/home/runner/work/arduino-cli/arduino-cli/internal/integrationtest/board/board_test.go:452
        	Error:      	Received unexpected error:
        	            	exit status 1
        	Test:       	TestBoardAttachWithoutSketchJson
--- FAIL: TestBoardAttachWithoutSketchJson (2.50s)
  • you must run a go mod tidy on all sub-projects to fix all the internal go.mod/go.sum
  • thinking about go.mod, we should also be able to get rid of this dependency: github.com/arduino/board-discovery so running a go mod tidy on the main repo should show it.

cli/board/board.go Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
@cmaglie
Copy link
Member

cmaglie commented Oct 20, 2022

Ah you already did it most of it, LOL. I should reload the page before starting a review :-)

@silvanocerza
Copy link
Contributor Author

Ah you already did it most of it, LOL. I should reload the page before starting a review :-)

I just pushed it really, you would have missed it in any case. 😅

By the way not sure about the sketch.json, the Sketch specifications docs still mention it and I didn't check other commands to see if it's still in use.

Regarding go mod tidy I found out that commands/daemon/term_example has a go.sum but no go.mod, so running it there fixes the root one.
It's also checked in CI check-outdated job, see this, but that's like checking root two times.

@cmaglie
Copy link
Member

cmaglie commented Oct 20, 2022

Regarding go mod tidy I found out that commands/daemon/term_example has a go.sum but no go.mod, so running it there fixes the root one.

I see, I think I forgot to commit the go.mod too, BTW since the example do not use any external library I think we can remove the extra go.sum and the check from the workflow.

@silvanocerza
Copy link
Contributor Author

I see dependencies checks are failing but I can't manage to run licensed locally. Would be great if someone could do it. <3

@per1234
Copy link
Contributor

per1234 commented Oct 20, 2022

Here you go Silvano: silvanocerza#5

@silvanocerza
Copy link
Contributor Author

Thanks @per1234, greatly appreciated. :)

@umbynos
Copy link
Contributor

umbynos commented Oct 21, 2022

Hello @silvanocerza, thanks a lot for this PR! Today with @per1234 @ubidefeo @cmaglie we spoke about this. In the end we decided to not completely deprecate the command, mainly because the users are using this and overall it only needs some love.
To summarize what we discussed:

  • drop the current implementation of the board attach cli command (done already in this PR)
  • the cli will still use the same sketch.json file for saving metadata
  • the board attach command now has to accept 3 flags --fqbn, --port, and optionally --protocol (regarding this there should already be all the helper functions in cli/arguments.

@silvanocerza
Copy link
Contributor Author

Sorry, no time to completely rework the command like that.
Feel free to keep working on this PR though.

@cmaglie
Copy link
Member

cmaglie commented Oct 25, 2022

I've pushed the implementation, but now I have my own review pending... I can approve the my changes by myself but doesn't look like a good plan mmmmmhhhh...

@umbynos umbynos self-requested a review October 25, 2022 14:59
@cmaglie cmaglie dismissed their stale review October 25, 2022 15:05

Take ownership of this PR

@cmaglie cmaglie force-pushed the remove-board-attach branch 3 times, most recently from be7e651 to b0375f0 Compare October 25, 2022 15:10
@cmaglie cmaglie changed the title [breaking] Remove board attach CLI command and gRPC interface [breaking] Fix board attach CLI command, and remove gRPC API Oct 25, 2022
cli/arguments/port.go Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
cli/board/attach.go Show resolved Hide resolved
@umbynos

This comment was marked as outdated.

@cmaglie cmaglie marked this pull request as draft November 4, 2022 16:15
@per1234 per1234 self-requested a review November 7, 2022 14:27
@cmaglie cmaglie force-pushed the remove-board-attach branch 2 times, most recently from 1a798d8 to 7e95d55 Compare November 7, 2022 15:28
@cmaglie cmaglie merged commit b1150e0 into arduino:master Nov 21, 2022
@umbynos umbynos added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: CLI Related to the command line interface labels Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: CLI Related to the command line interface topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
5 participants