interfaces/builtin: reduce duplication and remove cruft in Sanitize{Plug,Slot} #3619

zyga · Jul 25, 2017

This branch unifies how Sanitize{Plug,Slot} are implemented across all of the interfaces.
This removes a lot of copy-pasted code and adds some missing calls in other copy-pasted code.
Lastly, because all of the error messages are in a small set of four functions, there is much more
uniformity in how error messages look like.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

mvo5 · Jul 25, 2017

mvo5 reviewed Jul 25, 2017

View changes

Looks good and nice to see this duplication all consolidated into single helpers! Thanks for doing this work. A small question inline, otherwise good.

Do we need (explicit) tests for the new helpers?

They are tested indirectly but I'll add a small set of tests. Thanks for spotting that.

I just pushed some nice tests

codecov-io · Jul 25, 2017

Codecov Report

Merging #3619 into master will increase coverage by 0.04%.
The diff coverage is 86.88%.

@@            Coverage Diff             @@
##           master    #3619      +/-   ##
==========================================
+ Coverage   75.14%   75.19%   +0.04%     
==========================================
  Files         386      386              
  Lines       33693    33418     -275     
==========================================
- Hits        25318    25128     -190     
+ Misses       6551     6479      -72     
+ Partials     1824     1811      -13

Impacted Files	Coverage Δ
interfaces/builtin/lxd_support.go	`61.9% <ø> (-6.1%)`	⬇️
interfaces/builtin/bluez.go	`75.75% <ø> (+8.19%)`	⬆️
interfaces/builtin/maliit.go	`73.33% <ø> (+15.43%)`	⬆️
interfaces/builtin/location_control.go	`48.48% <ø> (+5.24%)`	⬆️
interfaces/builtin/uhid.go	`65.38% <ø> (+2.52%)`	⬆️
interfaces/builtin/unity8_pim_common.go	`73.17% <ø> (-2.34%)`	⬇️
interfaces/builtin/network_status.go	`71.42% <ø> (-0.8%)`	⬇️
interfaces/builtin/docker_support.go	`69.44% <ø> (+5.15%)`	⬆️
interfaces/builtin/pulseaudio.go	`37.03% <ø> (-8.13%)`	⬇️
interfaces/builtin/ofono.go	`61.53% <ø> (+5.72%)`	⬆️
... and 57 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 c81d104...f6fb74a. Read the comment docs.

zyga · Jul 27, 2017

After discussing this with @pedronis I'm going to move the responsibility of checking interface name from Sanitize{Plug,Slot} to the caller.

chipaca · Jul 28, 2017

chipaca approved these changes Jul 28, 2017

View changes

I'd make these interfaces public, to better document the thing (much as for example http.Flusher is public to document that). It adds a path to discovery of this feature.

Done.

pedronis · Jul 28, 2017

pedronis approved these changes Jul 28, 2017 • edited

View changes

lgtm, as I discussed we probably want to say "core snap" for the time being instead of "operating system snap".

Also I agree with Chipaca's comment, though it seems for the backends we really went the route of secret interfaces which I don't fully understand from a documenting POV.

zyga deleted the zyga:tweak/interface-common-code branch Jul 28, 2017

@@ -91,3 +91,39 @@ func udevUsbDeviceSnippet(subsystem string, usbVendor int64, usbProduct int64, k
          func udevSnapSecurityName(snapName string, appName string) string {
          	return fmt.Sprintf(`snap_%s_%s`, snapName, appName)
          }
+        +
+        +func ensureSlotIfaceMatch(iface interfaces.Interface, slot *interfaces.Slot) {

+        +			plug.Ref(), plug.Interface, iface.Name())
+        +	}
+        +	type sanitizer interface {
+        +		SanitizePlug(plug *Plug) error

snapcore/snapd

interfaces/builtin: reduce duplication and remove cruft in Sanitize{Plug,Slot} #3619

zyga commented Jul 25, 2017

zyga added some commits Jul 11, 2017

mvo5 reviewed Jul 25, 2017

View changes

codecov-io commented Jul 25, 2017 •

Edited 1 time

codecov-io
Jul 26, 2017

zyga commented Jul 27, 2017

zyga added some commits Jul 27, 2017

chipaca approved these changes Jul 28, 2017

View changes

pedronis approved these changes Jul 28, 2017 • edited

View changes

zyga added some commits Jul 28, 2017

zyga merged commit `1bb7d6d` into snapcore:master Jul 28, 2017
7 checks passed

7 checks passed

zyga deleted the zyga:tweak/interface-common-code branch Jul 28, 2017

snapcore/snapd

Join GitHub today

interfaces/builtin: reduce duplication and remove cruft in Sanitize{Plug,Slot} #3619

Conversation

zyga commented Jul 25, 2017

zyga added some commits Jul 11, 2017

All checks have passed

mvo5 reviewed Jul 25, 2017 View changes

mvo5 Jul 25, 2017

zyga Jul 25, 2017

zyga Jul 25, 2017

Some checks were not successful

codecov-io commented Jul 25, 2017 • edited Edited 1 time codecov-io Jul 26, 2017

Codecov Report

zyga commented Jul 27, 2017

zyga added some commits Jul 27, 2017

Some checks were not successful

Some checks were not successful

chipaca approved these changes Jul 28, 2017 View changes

chipaca Jul 28, 2017

zyga Jul 28, 2017

pedronis approved these changes Jul 28, 2017 • edited View changes

zyga added some commits Jul 28, 2017

Some checks were not successful

Some checks were not successful

All checks have passed

Hide details View details zyga merged commit 1bb7d6d into snapcore:master Jul 28, 2017 7 checks passed

7 checks passed

zyga deleted the zyga:tweak/interface-common-code branch Jul 28, 2017

mvo5 reviewed Jul 25, 2017

View changes

codecov-io commented Jul 25, 2017 •

Edited 1 time

codecov-io
Jul 26, 2017

chipaca approved these changes Jul 28, 2017

View changes

pedronis approved these changes Jul 28, 2017 • edited

View changes

zyga merged commit `1bb7d6d` into snapcore:master Jul 28, 2017
7 checks passed