Juju Charms Collection
juju-reports package

Merge lp://staging/~abentley/charms/precise/juju-reports/major-updates into lp://staging/~juju-qa/charms/precise/juju-reports/trunk

  1. Precise Pangolin (12.04)
  2. major-updates
  3. Merge into trunk
Proposed by Aaron Bentley
Status: Merged
Merged at revision: 25
Proposed branch: lp://staging/~abentley/charms/precise/juju-reports/major-updates
Merge into: lp://staging/~juju-qa/charms/precise/juju-reports/trunk
Diff against target: 351 lines (+158/-93)
9 files modified
config.yaml (+4/-0)
hooks/common.py (+142/-11)
hooks/config-changed (+2/-63)
hooks/database-relation-broken (+2/-2)
hooks/database-relation-changed (+2/-5)
hooks/database-relation-departed (+2/-2)
hooks/install (+1/-9)
hooks/start (+3/-0)
hooks/upgrade-charm (+0/-1)
To merge this branch: bzr merge lp://staging/~abentley/charms/precise/juju-reports/major-updates
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+218863@code.staging.launchpad.net

Commit message

Major updates to charm.

Description of the change

Major updates to charm.
1. all code is common code. Anything that runs python does
2. 'dist-clean' is introduced so that devs can turn of automatic "make dist-clean" and update much faster.
3. open-port and close-port are done as part of config now, not database-relation-changed
4. open / close port use the configured ports.
5. almost all use of in_juju_reports is removed.
6. restart is removed in favour of stop (before updating source) and start (after updating source).
7. LP key is written in update_source, not install hook.
8. port is closed when the service cannot operate.
9. Launchpad user is juju-qa-bot
10. start hook fails gracefully.
11. config is parsed as yaml instead of being looked up one-by-one. This also preserves value types.
12. use os.unlink instead of subprocess.call(['rm', ...])
13. stop early if mongodb url is not known.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

In update_source() you use a try/except block...I expected a context manger. The lines aren't wrong, I just expect context managers for resources in code that YOU write.

In that same function, I see we login as the bot. This has bothered me for some time. We don't logout. Should we? Am I paranoid. I ask because I am pondering a jenkin-juju-ci subordinate charm and I feel safer if the charm can login to get private branches, but logs out when done. The user is not left with powers that have no legitimate need.

In install_cronjob() file() is used. The function is deprecated. Does this work?
    open('/etc/cron.d/ubuntu', 'w').write(str(t))

review: Needs Information (code)
Revision history for this message
Aaron Bentley (abentley) wrote :

> In update_source() you use a try/except block...I expected a context manger.
> The lines aren't wrong, I just expect context managers for resources in code
> that YOU write.

I think I just figured that it was specific to the context, not something that could be generalized usefully. It also does sys.exit, which would be poor behaviour for a library function (and you can't return in a context manager).

> In that same function, I see we login as the bot.

It actually just sets the Launchpad user ID. It's not like Launchpadlib. It doesn't establish OAuth credentials or anything.

> This has bothered me for
> some time. We don't logout. Should we? Am I paranoid. I ask because I am
> pondering a jenkin-juju-ci subordinate charm and I feel safer if the charm can
> login to get private branches, but logs out when done. The user is not left
> with powers that have no legitimate need.

The actual secret is the SSH key, in this case. We can delete it after pulling, if we think that's useful. But it's security-by-obscurity-- the SSH key will still be accessible in the box, just not on the filesystem.

To embrace the principle of least privilege, we'd want to grant this machine access only to the one branch it needs. But that means a separate account, and I don't like doing that.

>
> In install_cronjob() file() is used. The function is deprecated. Does this
> work?
> open('/etc/cron.d/ubuntu', 'w').write(str(t))

Oh, it's not marked as deprecated in the Python 2 docs, but I see it's not in Python 3. Learn something every day. I'll change it.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you for explaining he situation and replacing the call to file(). r=me

review: Approve (code)
lp://staging/~abentley/charms/precise/juju-reports/major-updates updated
26. By Aaron Bentley

Use 'open' instead of 'file'.

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

to all changes: