interfaces: ensure that legacy interface methods are unused #3284

Merged
merged 4 commits into from May 10, 2017

Conversation

Projects
None yet
3 participants
Contributor

zyga commented May 9, 2017

The snapd tree has seen several iteration of the interface code. Each
iteration brought changes to how interfaces are written. Up until now
interfaces were strongly typed, with each interface method being defined
in the Interface go interface. For a few months though they became
loosely typed as each security back-end detects and uses a subset of
methods that are specific to itself.

As another change is approaching, the interface methods will change
again. The way some of the unit tests are written may allow tests to
pass as the existing method are tested. At runtime those methods would
not be used though, and this is certainly undesired.

To avoid potential issues this patch adds a new unit test that looks at
all the known interfaces and makes sure that none of them define any of
the unused methods. This way each API transition can be accompanied by a
simple test extension. This should ensure that no merged pull request
(especially an older one) accidentally brings stale APIs into the tree.

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

zyga added some commits Apr 6, 2017

interfaces: ensure that legacy interface methods are unused
The snapd tree has seen several iteration of the interface code. Each
iteration brought changes to how interfaces are written. Up until now
interfaces were strongly typed, with each interface method being defined
in the Interface go interface. For a few months though they became
loosely typed as each security back-end detects and uses a subset of
methods that are specific to itself.

As another change is approaching, the interface methods will change
again. The way some of the unit tests are written may allow tests to
pass as the existing method are tested. At runtime those methods would
not be used though, and this is certainly undesired.

To avoid potential issues this patch adds a new unit test that looks at
all the known interfaces and makes sure that none of them define any of
the unused methods. This way each API transition can be accompanied by a
simple test extension. This should ensure that no merged pull request
(especially an older one) accidentally brings stale APIs into the tree.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: detect legacy auto-connect function
This patch extends the legacy API detector to look for the
LegacyAutoConnect method that is no longer used by anything at all.

The same patch also removes the method from i2c and ofono interfaces.

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

That's super-useful to have, thank you! One minor nitpick, please address before merging. Approving right away.

+ bogus := true
+ for _, definer := range allGoodDefiners {
+ if reflect.TypeOf(iface).Implements(definer) {
+ bogus = false
@stolowski

stolowski May 9, 2017

Contributor

Micro-optization: break once you found a matching one ;)

@zyga

zyga May 9, 2017

Contributor

Done :)

zyga added some commits May 9, 2017

interfaces/builtin: break early if we find a match
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: add extra comment
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

mvo5 approved these changes May 10, 2017

Looks good

@mvo5 mvo5 merged commit 5ced19c into snapcore:master May 10, 2017

7 checks passed

artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:feature/no-funny-interfaces branch May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment