interfaces: validate plug/slot uniqueness #3153

Merged
merged 13 commits into from Apr 11, 2017

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Apr 7, 2017

This branch improves validation of changes made to the interfaces repository. In the past this was done
at snap.Info level, specifically via snap.Validate but this was ignoring additional changes caused by
adding implicit plug or slot names. This has caused a bug where the core snap would define some plugs
directly in the YAML file (specifically network-bind plug) and and than would gain implicit network-bind slot through the implicit mechanism. Now direct additions of plugs and slots are also enforcing the uniqueness constraint.

NOTE: This branch might only be clean to land after the new core snap is released.

zyga added some commits Apr 7, 2017

interfaces: don't call plug.Snap.Name() repeatedly
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces: detect clashing slots when adding new plugs
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces: don't call slot.Snap.Name() repeatedly
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces: detect clashing slots when adding new plugs
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces: validate snaps added to the repository
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: add function for renaming plugs
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: rename clashing plugs on core snap
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga added this to the 2.24 milestone Apr 7, 2017

Contributor

zyga commented Apr 7, 2017

This can only land after we land #3154

Contributor

zyga commented Apr 10, 2017

This branch depends on #3154

mvo5 approved these changes Apr 10, 2017

Looks good!

LGTM. Just a suggestion for the messaging:

interfaces/repo.go
- if _, ok := r.plugs[plug.Snap.Name()][plug.Name]; ok {
- return fmt.Errorf("cannot add plug, snap %q already has plug %q", plug.Snap.Name(), plug.Name)
+ if _, ok := r.plugs[snapName][plug.Name]; ok {
+ return fmt.Errorf("cannot add plug %q, snap %q already has a plug with that name", plug.Name, snapName)
@niemeyer

niemeyer Apr 10, 2017

Contributor

From the user perspective, there's no plug or slot being added. I suggest reporting this as:

"snap %q has plugs conflicting on name %q"
"snap %q has plug and slot conflicting on name %q"

and so on.

@zyga

zyga Apr 10, 2017

Contributor

Aha, good point, this is just internal thing. I'll update this.

@zyga

zyga Apr 10, 2017

Contributor

Done

zyga added some commits Apr 10, 2017

interfaces: tweak error messages on clashes
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Almost there. Per prior review:

interfaces/repo.go
- if _, ok := r.plugs[plug.Snap.Name()][plug.Name]; ok {
- return fmt.Errorf("cannot add plug, snap %q already has plug %q", plug.Snap.Name(), plug.Name)
+ if _, ok := r.plugs[snapName][plug.Name]; ok {
+ return fmt.Errorf("snap %q has plug and slot conflicting on name %q", snapName, plug.Name)
@niemeyer

niemeyer Apr 10, 2017

Contributor

In this case it's plug and plug. Per the original comment here:

"snap %q has plugs conflicting on name %q"
@zyga

zyga Apr 10, 2017

Contributor

Ah, sorry, missed this detail. Same below. Correcting now.

interfaces/repo.go
- if _, ok := r.slots[slot.Snap.Name()][slot.Name]; ok {
- return fmt.Errorf("cannot add slot, snap %q already has slot %q", slot.Snap.Name(), slot.Name)
+ if _, ok := r.slots[snapName][slot.Name]; ok {
+ return fmt.Errorf("snap %q has plug and slot conflicting on name %q", snapName, slot.Name)
@niemeyer

niemeyer Apr 10, 2017

Contributor

Similarly:

"snap %q has slots conflicting on name %q"
@zyga

zyga Apr 10, 2017

Contributor

Corrected as above.

interfaces: correct wording when plug/plug or slot/slot clash
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga added some commits Apr 10, 2017

many: remove RenamePlug patch (wrong tree)
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@mvo5 mvo5 modified the milestones: 2.25, 2.24 Apr 11, 2017

@zyga zyga merged commit 8ac2f82 into snapcore:master Apr 11, 2017

6 checks passed

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:unique-plug-slot-names branch Apr 11, 2017

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