Merge lp://qastaging/~veebers/ueqa-code-proposals/indicator-power into lp://qastaging/ueqa-code-proposals

Proposed by Christopher Lee
Status: Merged
Approved by: Leo Arias
Approved revision: 16
Merged at revision: 13
Proposed branch: lp://qastaging/~veebers/ueqa-code-proposals/indicator-power
Merge into: lp://qastaging/ueqa-code-proposals
Diff against target: 323 lines (+209/-59)
2 files modified
proposals/indicator-power.py (+103/-38)
proposals/indicator-power.rst (+106/-21)
To merge this branch: bzr merge lp://qastaging/~veebers/ueqa-code-proposals/indicator-power
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Allan LeSage (community) Approve
Leo Arias (community) Approve
Review via email: mp+242169@code.qastaging.launchpad.net

Commit message

Add indicator-power testability proposal incl. API code.

Description of the change

Add indicator-power testability proposal incl. API code.

To post a comment you must log in.
Revision history for this message
Christopher Lee (veebers) wrote :

Note that after some conversation with pitti that this needs a minor tweak that I will add first thing in the morning (as well as any other review related comments that might appear in the mean time).

Tweak is regarding stopping/restarting indicator-power and ensuring that we don't poison the initctl env var space so new processes don't look at the mock bus.

Revision history for this message
Leo Arias (elopio) wrote :

As this files will probably be copied by the developers, I think we should also work a little towards getting them nicely formatted and to be consistent with the rest of our projects. So I'll also make some small style comments.

review: Needs Information
Revision history for this message
Allan LeSage (allanlesage) wrote :

Ok this is good; got some informal comments from charles concerning the depth of the investigation you've already done :) . I do think it's important to try to prevent future blockages, especially if we're offering specific technical suggestions. Do we want a pitti review?

Finally I expended some time on UI helpers for datetime and I wonder if we'll need something similar here, surprised Leo isn't asking for: I think the plan is to use the pattern he demonstrates with indicator-messages and adapt, so this can land but we might return to this if he has further suggestions.

So for fixes (because you offered) at least a test and comment on trying this under autopilot.

review: Needs Fixing
7. By Christopher Lee

Update and extend proposal as per MP comments.

Revision history for this message
Allan LeSage (allanlesage) wrote :

More comments, I think you'll need more UI flavor but I'll let Leo chime in on that. Just checked in with charles on the private-bus-env var idea and it sounds plausible BTW, willing to change? <==(request for information)

review: Needs Information
8. By Christopher Lee

Modifications and updates as per alesages comments

9. By Christopher Lee

Slight update as per Leo's comment, will need to extend it

10. By Christopher Lee

Remove the word 'testability'

Revision history for this message
Leo Arias (elopio) wrote :

There are some typos, makred in the diff comments. Other than that, it looks good to show to the manager. So, I'm leaving my approval here.

review: Approve
11. By Christopher Lee

Typo fixes as pointed out by elopio.

12. By Christopher Lee

Further minor fixes

Revision history for this message
Christopher Lee (veebers) wrote :

> There are some typos, makred in the diff comments. Other than that, it looks
> good to show to the manager. So, I'm leaving my approval here.
Sweet, thanks Leo. I have fixed the typos.

13. By Christopher Lee

Update comment re: packaging

14. By Christopher Lee

Another minor change

Revision history for this message
Allan LeSage (allanlesage) :
review: Approve
15. By Christopher Lee

Merge trunk

16. By Christopher Lee

Update UI helpers to reference common

Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches