Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/snap: add 'debug paths' command #5459

Merged
merged 7 commits into from
Jul 6, 2018

Conversation

bboozzoo
Copy link
Collaborator

@bboozzoo bboozzoo commented Jul 4, 2018

Add snap debug paths command that shows paths that have been detected for the
system. This currently includes:

  • snap mount dir
  • snap bin dir
  • distro libexecdir

Add `snap debug paths` command that shows paths that have been detected for the
system.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo bboozzoo added the Simple 😃 A small PR which can be reviewed quickly label Jul 4, 2018
w := tabWriter()
defer w.Flush()

// TODO: include paths reported by snap-confine
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to have a way to interrogate s-c get back whatever was passed as --with-snap-mount-dir, --libexecdir and maybe --with-32bit-libdir and include it in the output here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting, yes (for as long as there are hard-coded assumptions in snap-confine)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be better to fix it instead of interrogating it, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No disagreements there :)

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the concept, this is very very nice IMO

@codecov-io
Copy link

codecov-io commented Jul 4, 2018

Codecov Report

Merging #5459 into master will increase coverage by 0.01%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5459      +/-   ##
==========================================
+ Coverage   79.01%   79.02%   +0.01%     
==========================================
  Files         512      513       +1     
  Lines       38674    38701      +27     
==========================================
+ Hits        30557    30584      +27     
  Misses       5664     5664              
  Partials     2453     2453
Impacted Files Coverage Δ
cmd/snap/cmd_paths.go 88.23% <88.23%> (ø)
overlord/hookstate/hookmgr.go 75.24% <0%> (+0.97%) ⬆️
overlord/devicestate/devicestate.go 79.72% <0%> (+1.46%) ⬆️

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 eb838bf...7c67b7d. Read the comment docs.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine - I'm curious what the use-case is? I also wonder if it should be part of sysinfo if its something that people expect to discover via the API (but that depends on the use-case of course).

@zyga
Copy link
Collaborator

zyga commented Jul 5, 2018

@bboozzoo one last request, please add a simple spread test.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to expose this. Some suggestions:

w := tabWriter()
defer w.Flush()

// TODO: include paths reported by snap-confine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per note above, let's please not do this and drop the TODO. For two reasons:

  • The snap-confine bundled with snapd should use the same paths as the snapd and snap binary that shipped with it, so we can interrogate those instead for the same result (note we're not querying snapd either here).
  • We should fix snap-confine to use paths dynamically if/when it needs to.

{"snap-bin-dir", dirs.SnapBinariesDir},
{"distro-libexec", dirs.DistroLibExecDir},
} {
fmt.Fprintf(w, "%s:\t%s\n", p.name, p.path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A format like this might be better here:

SNAPD_MOUNT=...
SNAPD_BIN=...
SNAPD_LIBEXEC=...

This allows usage in shell for analysis, and it also uses the $SNAPD_ prefix, which is what we generally have for snapd-related content. We usually reserve the $SNAP_ prefix for things which are for a snap, with $SNAP being the path to the snap directory under /snap. We also don't use DIR for $SNAP*, so we don't need it here either.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
… output

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, on suggestion to kill the tabWriter, otherwise ready to go.

return ErrExtraArgs
}

w := tabWriter()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a tabWriter anymore, do we? The new code is not writing tabs anymore AFAICT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}

func (s *SnapSuite) TestPathsFedora(c *C) {
restore := release.MockReleaseInfo(&release.OS{ID: "fedora"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test(s)

snap debug paths | MATCH "^SNAPD_LIBEXEC=${LIBEXECDIR}/snapd$"

# double check we can eval it as shell
eval "$(snap debug paths)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo bboozzoo merged commit 3e52491 into snapcore:master Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
5 participants