Merge lp://qastaging/~cjohnston/qa-dashboard/auto-update-assets into lp://qastaging/qa-dashboard

Proposed by Chris Johnston
Status: Merged
Approved by: Joe Talbott
Approved revision: 698
Merged at revision: 756
Proposed branch: lp://qastaging/~cjohnston/qa-dashboard/auto-update-assets
Merge into: lp://qastaging/qa-dashboard
Diff against target: 63 lines (+59/-0)
1 file modified
scripts/update_assets.py (+59/-0)
To merge this branch: bzr merge lp://qastaging/~cjohnston/qa-dashboard/auto-update-assets
Reviewer Review Type Date Requested Status
Andy Doan (community) Approve
PS Jenkins bot continuous-integration Approve
Chris Johnston Needs Resubmitting
Review via email: mp+197435@code.qastaging.launchpad.net

Commit message

Automagically update lp:qa-dashboard/assets when new/modified assets exist; update assets_revision with the new revision number.

Description of the change

This branch is creating a script which will check staticfiles to see if they have been updated. If there are new/modified files, update_assets.py will create a new directory in lp:qa-dashboard/assets with the revision number of the next lp:qa-dashboard commit, copy the staticfiles and then push them up to LP for inclusion on assets.ubuntu.com

It will also modify the assets_revision.py file to reflect which directory the dashboard should be looking for assets files in assets.ubuntu.com

This script will require that lp:qa-dashboard/assets live at qa-dashboard/assets for tarmac

To post a comment you must log in.
Revision history for this message
Andy Doan (doanac) wrote :

14 +regex = re.compile(ur'0 static files copied,')
22 + if not regex.search(output):

should we make this check for >0 amount of files copied? ie - could this code path get hit if an exception were thrown and the program didn't print anything? or maybe search for:

 regex = re.compile(ur'\d+ static files copied,')
 match = regex.search(output)
 if not match:
    print("WTF???")
 elif output[0] != '0':
    print("update")

Revision history for this message
Andy Doan (doanac) wrote :

why str(new_dir) ? isn't it already a string?
32 + subprocess.check_call(['bzr', 'mkdir', str(new_dir)], cwd=assets_dir)

Revision history for this message
Andy Doan (doanac) wrote :

How about using shutil to copy a directory rather than this:

33 + subprocess.check_call(
34 + [
35 + 'cp',
36 + '-r',
37 + str(static_dir),
38 + str(new_dir),
39 + ],
40 + cwd=assets_dir,
41 + )

Revision history for this message
Andy Doan (doanac) wrote :

57 + try:
58 + from bzrlib.branch import Branch
59 + branch = Branch.open_containing('.')[0]
60 + bzr_revno = '%s' % (int(branch.revno())+1)
61 + except:
62 + bzr_revno = 'unknown'
63 +

I usually like using the API, but in this case we mask a potential import-error (ie python-bzrlib) isn't installed with some generic error (i think). I'd either put that import at the top of the file. or just use subprocess like you do everywhere else for your bzr stuff with:

 return int(subprocess.check_output(['bzr', 'revno'], cwd=...))

Revision history for this message
Andy Doan (doanac) wrote :

nitpicking, but its nicer form to use context managers:

71 + f = open(file_name, "w")
72 + f.write("""revision = '%s'""" % revno)
73 + f.close()

with open(file_name, 'w') as f:
    f.write(.......)

and one less line of code

694. By Chris Johnston

Update per review

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:693
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/266/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/266/rebuild

review: Approve (continuous-integration)
695. By Chris Johnston

update getting revno per review

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:695
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/267/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/267/rebuild

review: Approve (continuous-integration)
696. By Chris Johnston

Update file write per review

697. By Chris Johnston

Update copy method per review

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:697
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/268/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/268/rebuild

review: Approve (continuous-integration)
698. By Chris Johnston

Update regex check per review

Revision history for this message
Chris Johnston (cjohnston) :
review: Needs Resubmitting
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:698
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/269/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/269/rebuild

review: Approve (continuous-integration)
Revision history for this message
Andy Doan (doanac) wrote :

looks good. i just have one minor nit - you can take it or leave it:

54 + if os.path.exists(file_name):
55 + os.remove(file_name)

That code isn't need open(file_name, 'w') will truncate the file

Revision history for this message
Andy Doan (doanac) :
review: Approve

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