canonical / multipass

Multipass orchestrates virtual Ubuntu instances
https://multipass.run
GNU General Public License v3.0
7.63k stars 635 forks source link

Fix bridging authorization #3395

Closed luis4a0 closed 5 months ago

luis4a0 commented 6 months ago

The prompt asking the user for authorization to disrupt networking to create a bridge was not working. This PR fixes issue 1 of this review.

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 88.67%. Comparing base (f51d0cc) to head (cee21ad).

Files Patch % Lines
src/daemon/daemon.cpp 91.30% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3395 +/- ## ========================================== + Coverage 88.63% 88.67% +0.04% ========================================== Files 252 253 +1 Lines 13949 13993 +44 ========================================== + Hits 12363 12408 +45 + Misses 1586 1585 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

townsend2010 commented 6 months ago

I will add that aside from the wrong string that I pointed out, it does work as intended. :slightly_smiling_face:

ricab commented 6 months ago

I agree with @townsend2010. The handling for the bridging prompt is too specific for the generic set code. I would rather see a generic scheme to accommodate generic confirmation prompts, ideally applying to all commands and covering other cases of confirmation/choice prompts (e.g. auto snapshot prompt in restore). That said, I am not sure that this is the right time and place for that... Maybe there's a more accessible middle ground for now? I wouldn't mind seeing a proposal for what shape that could take. WDYT @luis4a0?

luis4a0 commented 6 months ago

Hey @townsend2010, @ricab! You are both right, we can do better in the prompt. Something which comes to my mind is to change the BridgePrompter to a sort of YesNoPrompter, which accepts a custom string to be displayed as prompt. WDYT?

OTOH, @townsend2010, that true in the string comes from a wrong call. I'll fix it.

Thanks to both for the comments and the review!

townsend2010 commented 6 months ago

I think a more generic Prompter type is better. Thinking about it some, I can envision a Yes/No prompter, essentially something that accepts a bool. I also envision a prompter that would accept a string. So yes, for this PR, I think switching to a YesNoPrompter that accepts a custom string used for displaying to the user is good.

Also, I'm thinking instead of the "custom" failure handling, using the StreamingCallback may be more efficient here. It's what I used to implement the SMB mount password handling in Windows and seemed to work well for client/daemon ping ponging.

luis4a0 commented 6 months ago

@townsend2010, fixed the true in the prompt. Now the displayed network is correct.

luis4a0 commented 5 months ago

Hey @townsend2010! I addressed your comments and improved testing. This is ready for a new review!