cmd,client,daemon: expose "force devmode" in sysinfo #3542

Merged
merged 2 commits into from Jul 6, 2017

Conversation

Projects
None yet
6 participants
Contributor

zyga commented Jun 29, 2017

The GNOME software center would like to show appopriate icons if a snap
application is confined and while each snap shows its confinement
information the whole system may be unable to offer effective
confinement. This patch adds a "confinement" key to the sysinfo
interface and switches the existing "snap debug confinement" command to
use it, rather than implement the logic in the client.

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

@zyga zyga requested a review from chipaca Jun 29, 2017

Contributor

robert-ancell commented Jun 29, 2017

Looks good, thanks!

daemon/api.go
+ // in place how we can dynamically retrieve these information from
+ // snapd we will use this here.
+ if release.ReleaseInfo.ForceDevMode() {
+ m["confinement"] = "none"
@chipaca

chipaca Jun 29, 2017

Member

is it really "none", or is it "only seccomp"?

@zyga

zyga Jun 29, 2017

Contributor

It's none for the POV of the user. We don't want to drill down to the "this or that" details.

@Conan-Kudo

Conan-Kudo Jun 29, 2017

Contributor

I don't know if it's a great idea to make it like that. It's a more negative lie than the current one. Can we say something like "basic" or "full" confinement?

@zyga

zyga Jun 29, 2017

Contributor

We can evolve this but I just kept the values that we have already.

@mvo5

mvo5 Jun 30, 2017

Collaborator

The latest idea was "partial" or "strict"

Contributor

robert-ancell commented Jun 29, 2017

This field is an enum right, rather than a free-form string? (Asking for documentation and snapd-glib implementation purposes).

codecov-io commented Jun 29, 2017

Codecov Report

Merging #3542 into master will increase coverage by 0.02%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3542      +/-   ##
==========================================
+ Coverage    76.8%   76.83%   +0.02%     
==========================================
  Files         379      379              
  Lines       26285    26290       +5     
==========================================
+ Hits        20189    20199      +10     
+ Misses       4304     4297       -7     
- Partials     1792     1794       +2
Impacted Files Coverage Δ
client/client.go 80.88% <ø> (ø) ⬆️
daemon/api.go 73.26% <100%> (+0.07%) ⬆️
cmd/snap/cmd_confinement.go 63.63% <60%> (+43.63%) ⬆️
cmd/snap/cmd_aliases.go 96% <0%> (+2%) ⬆️

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 b7b3789...89729aa. Read the comment docs.

Contributor

robert-ancell commented Jun 29, 2017

This is required for Fedora because without this change GNOME Software shows all snaps as confined. As this is not true in Fedora they want this information so snaps are correctly marked as not confined (the default for RPM packages).

Looks good, but I thnk we want to tweak the wording.

daemon/api.go
+ // in place how we can dynamically retrieve these information from
+ // snapd we will use this here.
+ if release.ReleaseInfo.ForceDevMode() {
+ m["confinement"] = "none"
@chipaca

chipaca Jun 29, 2017

Member

is it really "none", or is it "only seccomp"?

@zyga

zyga Jun 29, 2017

Contributor

It's none for the POV of the user. We don't want to drill down to the "this or that" details.

@Conan-Kudo

Conan-Kudo Jun 29, 2017

Contributor

I don't know if it's a great idea to make it like that. It's a more negative lie than the current one. Can we say something like "basic" or "full" confinement?

@zyga

zyga Jun 29, 2017

Contributor

We can evolve this but I just kept the values that we have already.

@mvo5

mvo5 Jun 30, 2017

Collaborator

The latest idea was "partial" or "strict"

@mvo5 mvo5 requested a review from niemeyer Jun 30, 2017

mvo5 approved these changes Jun 30, 2017

Contributor

robert-ancell commented Jul 5, 2017

Anything blocking this other than the CI build?

Contributor

zyga commented Jul 6, 2017

@robert-ancell I think it's just CI now, I think tests will pass now, the Linode problem was fixed yesterday by Gustavo.

cmd,client,daemon: expose "force devmode" in sysinfo
The GNOME software center would like to show appopriate icons if a snap
application is confined and while each snap shows its confinement
information the whole system may be unable to offer effective
confinement. This patch adds a "confinement" key to the sysinfo
interface and switches the existing "snap debug confinement" command to
use it, rather than implement the logic in the client.

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

zyga commented Jul 6, 2017

I fixed the tests, it should go green soon.

daemon: rename "none" confinement to "partial"
There is no way to have "no" confinement as many backends are used
at all times. The names should reflect this.

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

@zyga zyga merged commit e3c0003 into snapcore:master Jul 6, 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/confinement-flag-on-sysinfo branch Jul 6, 2017

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