overlord/ifacestate: remove "old-conn" from connect/undo connect handlers

[stolowski]

Remove "old-conn" attribute from connect and its undo handler.
It was initially implemented to support re-connecting over existing connection, in which case undo would restore the previous state of the connection. The idea of re-connecting existing connection was however dropped and re-connecting is explicitly prevented in ifacestate.Connect(..) method.

Note, "old-conn" is also used in disconnect/disconnect-undo handlers, but it's required there.

[niemeyer]

Not disagreeing, but can we have some more background for the choice of dropping undo? What are the consequences of doing that, and what is the rationale for not attempting to undo a disconnection?

[pedronis]

@niemeyer this code precisely doesn't touch disconnect or its undo as @stolowski mentions in the description, it's about the case of undoing a form of connect (connect for an already existing connection) that we agreed in the end not to use/support

[pedronis]

looks correct to me

[stolowski]

@niemeyer Yes, as Samule said, it's about undo for connect only. We simply never run connect handler over existing connection; connect can only be run if there is no connection yet, so there is no point in storing old-connection in the task; undo'ing connect is just removing connection from the state (from conns). And of course as part of connect undo we also run disconnect- and unprepare- hooks.

[niemeyer]

Makes sense, thanks.

[codecov-io]

Codecov Report

Merging #5718 into master will increase coverage by 0.34%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5718      +/-   ##
==========================================
+ Coverage   78.98%   79.32%   +0.34%     
==========================================
  Files         525      528       +3     
  Lines       40105    40776     +671     
==========================================
+ Hits        31676    32345     +669     
+ Misses       5856     5852       -4     
- Partials     2573     2579       +6

Impacted Files	Coverage Δ
overlord/ifacestate/handlers.go	64.07% <100%> (+0.49%)	⬆️
httputil/logger.go	77.27% <0%> (-2.28%)	⬇️
overlord/hookstate/hookmgr.go	73.55% <0%> (-0.97%)	⬇️
image/image.go	71.42% <0%> (-0.44%)	⬇️
httputil/client.go	100% <0%> (ø)
cmd/snap/cmd_version_linux.go	33.33% <0%> (ø)
overlord/configstate/proxyconf/proxy.go	75% <0%> (ø)
store/store.go	87.9% <0%> (+5.98%)	⬆️
overlord/overlord.go	88.07% <0%> (+6.29%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4286ea...e7961f0. Read the comment docs.