Merge lp:~milo/launchpad-work-items-tracker/bug996948 into lp:~linaro-automation/launchpad-work-items-tracker/linaro

Proposed by Milo Casagrande
Status: Merged
Merged at revision: 343
Proposed branch: lp:~milo/launchpad-work-items-tracker/bug996948
Merge into: lp:~linaro-automation/launchpad-work-items-tracker/linaro
Diff against target: 28 lines (+10/-2)
1 file modified
collect (+10/-2)
To merge this branch: bzr merge lp:~milo/launchpad-work-items-tracker/bug996948
Reviewer Review Type Date Requested Status
Paul Sokolovsky Approve
Loïc Minier Pending
Linaro Infrastructure Pending
Review via email: mp+128654@code.launchpad.net

This proposal supersedes a proposal from 2012-10-09.

Description of the change

Merge proposal in order to fix the integrity error we receive from status.l.o.

Changes done:
- catch the integrity error and print warning with name of duplicated Blueprint
- skip the Blueprint and continue, otherwise script will fail later in the code due to variable not being set

To post a comment you must log in.
Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

I'm not sure this should be a warning. This condition is clearly makes the application not worked as intended (not to process its work items), so this should be an error. Definitely not a fatal, so continuing is nice. "Needs fixing"

Other thing I was thinking about was showing both old and new blueprints - while the current patch is nice and easy, in real world situation, I guess a stakeholder would like to know both offending blueprints. I'd defer to stakeholders though (would definitely require more time to implement).

review: Needs Fixing
Revision history for this message
Milo Casagrande (milo) wrote :

> I'm not sure this should be a warning. This condition is clearly makes the
> application not worked as intended (not to process its work items), so this
> should be an error. Definitely not a fatal, so continuing is nice. "Needs
> fixing"

Sure, not a problem.
Am not sure about including the error too though.

> Other thing I was thinking about was showing both old and new blueprints -
> while the current patch is nice and easy, in real world situation, I guess a
> stakeholder would like to know both offending blueprints. I'd defer to
> stakeholders though (would definitely require more time to implement).

Something more can be written, for sure. Getting also the "old" Blueprint means we will have to perform another DB lookup though, since we only have the "new-duplicated" Blueprint, not the one already in the DB. I can easily add the Blueprint URL, at least for the new one, since it should be the one to be modified at least.

344. By Milo Casagrande

Used error logger, added URL to Blueprint.

Revision history for this message
Milo Casagrande (milo) wrote :

Ok, I fixed the logger usage, and manage also to retrieve the URL of at least the "new-duplicated" Blueprint.

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

Great, thanks for investigating that!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'collect'
2--- collect 2012-08-08 12:38:10 +0000
3+++ collect 2012-10-09 13:12:22 +0000
4@@ -15,6 +15,7 @@
5 import sys
6 import urllib
7 import urlparse
8+import sqlite3
9
10 from email.mime.text import MIMEText
11
12@@ -223,8 +224,15 @@
13 in_complexity_block = False
14 work_items = []
15
16- model_bp = collector.store.find(
17- Blueprint, Blueprint.name == bp.name).one()
18+ try:
19+ model_bp = collector.store.find(Blueprint,
20+ Blueprint.name == bp.name).one()
21+ except sqlite3.IntegrityError:
22+ logger.error('Duplicated Blueprint found: %s. It will not be '
23+ 'considered. Blueprint URL is: %s' % bp.name)
24+ logger.error('Blueprint URL is: %s' % bp.specification_url)
25+ return
26+
27 assert model_bp is not None, \
28 "Asked to process workitems of %s when it is not in the db" % bp.name
29

Subscribers

People subscribed via source and target branches