Snappy

Merge lp://qastaging/~jamesodhunt/snappy/log-commands-that-change-system-state into lp://qastaging/~snappy-dev/snappy/snappy-moved-to-github

log-commands-that-change-system-state
Merge into snappy-moved-to-github

Proposed by James Hunt on 2015-03-27

Status:	Work in progress
Proposed branch:	lp://qastaging/~jamesodhunt/snappy/log-commands-that-change-system-state
Merge into:	lp://qastaging/~snappy-dev/snappy/snappy-moved-to-github
Diff against target:	351 lines (+120/-9) 12 files modified cmd/snappy-go/cmd_booted.go (+12/-1) cmd/snappy-go/cmd_hwassign.go (+8/-1) cmd/snappy-go/cmd_hwunassign.go (+8/-1) cmd/snappy-go/cmd_rollback.go (+8/-1) cmd/snappy-go/cmd_update.go (+4/-0) partition/partition.go (+16/-0) partition/partition_test.go (+3/-0) snappy/click.go (+4/-0) snappy/install.go (+9/-1) snappy/snapp.go (+9/-1) snappy/systemimage.go (+31/-3) snappy/systemimage_test.go (+8/-0)
To merge this branch:	bzr merge lp://qastaging/~jamesodhunt/snappy/log-commands-that-change-system-state
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
John Lenton (community)	2015-03-27	Disapprove on 2015-04-13
Michael Vogt (community)		Needs Information on 2015-03-31
Sergio Schvezov	2015-04-10	Pending
Review via email: mp+254405@code.qastaging.launchpad.net

Description of the change

* Added logging calls for commands that modify the system state.
* partition/partition.go:
- Added RootfsLabel() and OtherRootfsLabel() implementations to enable
rich log messages.
* Upated tests for new functions.

This branch was rather awkward given the way we handle clicks and s-i parts differently. I've tried to keep the log calls as "high up" in the calling stack as possible, but on occasion you'll see that I had to dive down a little lower than desired to fit in with the current code structure.

Note too that there is a small amount of duplication in the logging output:

a) Installing and then removing a snap gives the following:

INFO:snappy:/home/james/go/src/launchpad.net/snappy/snappy/click.go:735:Installed webdm (version 0.1)
INFO:snappy:/home/james/go/src/launchpad.net/snappy/snappy/install.go:92:Installed com.ubuntu.snappy.webdm (version 0.1)
INFO:snappy:/home/james/go/src/launchpad.net/snappy/snappy/snapp.go:322:Removed webdm (version 0.1)

Which of the 2 "Installed" message do we want to keep?

b) s-i parts:

INFO:snappy:/home/james/go/src/launchpad.net/snappy/snappy/systemimage.go:202:Updated system-b partition to version 346
INFO:snappy:/home/james/go/src/launchpad.net/snappy/snappy/systemimage.go:142:Updated system to boot from system-b partition on next boot
INFO:snappy:/home/james/go/src/launchpad.net/snappy/cmd/snappy-go/cmd_update.go:66:Installed ubuntu-core (version 346)

I wonder if we could retain all the s-i messages since:

a) An installed message should be provided for all parts.
b) The other 2 Updated messages are useful as they provide details of the partition (essential IMHO).

Revision history for this message

John Lenton (chipaca) wrote on 2015-03-30:

I don't have answers to your questions (is an MP the right place to ask the questions? are you really asking, or is it your way of exploring future work?). But the branch looks good.

review: Approve

Revision history for this message

Michael Vogt (mvo) wrote on 2015-03-31:

Thanks for working on this!

I started reviewing this branch but I think this needs a quick sync with Sergio before I continue. By doing most of the logging in cmd/snappy webdm will have to duplicate that code.

It seems worthwhile to talk about moving the cmd/ bits into snappy/partition so that he automatically benefits.

I still have some inline comments, it seems like there is value in extracting some of the common pattern(s) into helpers.

review: Needs Information

Revision history for this message

John Lenton (chipaca) wrote on 2015-04-13:

So, talking with mvo and sergiusens, we need to change the approach entirely: we need to change cmd_* to be as simple as possible, and pass loggers (and the other bits) into the library for the library itself to do the logging on the passed in loggers.

review: Disapprove

Unmerged revisions

268. By James Hunt on 2015-03-27: * Sync with lp:snappy.
267. By James Hunt on 2015-03-27: * Added logging calls for commands that modify the system state.
* partition/partition.go:
- Added RootfsLabel() and OtherRootfsLabel() implementations to enable
rich log messages.
* Upated tests for new functions.