Fix bridging authorization

[luis4a0]

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]

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 ⚠️

Additional details and impacted files

@@            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     

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

[townsend2010]

Hi @luis4a0! Thanks for this!

It appears the output is not quite correct when it prompts for allowing to create the bridge. Here is what I see:

$ multipass set local.test-issue-1.bridged=true
Multipass needs to create a bridge to connect to true.
This will temporarily disrupt connectivity on that interface.

Do you want to continue (yes/no)?

The first line says Multipass needs to create a bridge to connect to true. and notice that it ends with true which is not correct.

Also, I really feel this is overloading generic commands/RPC in a very specific way which is kind of changing this interface to bend to the needs of asking for bridging authorization. I feel the asking for bridging authorization should somehow be decoupled somewhat from all of this or at least make it to where the prompting could be more generalized. I think we need to give this some more thought and I would also like @ricab's opinion on this as well.

[townsend2010]

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

[ricab]

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]

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]

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]

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

[townsend2010]

Hi @luis4a0!

Thanks for the changes you've done. I have one request below and then I think we are good pending your investigation as to why the test coverage fell so much.

[townsend2010]

Hey @luis4a0!

Thanks for this. Just one more comment below regarding test coverage. I'll await your reply 🙂

[townsend2010]

Hey @luis4a0,

I answered your reply about more coverage and have some more questions below regarding the tests.

[luis4a0]

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

[townsend2010]

Hey @luis4a0!

This looks much better! Thanks! I'm approving, but I will wait on CI to complete before adding it to the merge queue.