MAAS

Merge lp://staging/~mpontillo/maas/iprange-notifications--bug-1393936 into lp://staging/~maas-committers/maas/trunk

  1. iprange-notifications--bug-1393936
  2. Merge into trunk
Proposed by Mike Pontillo
Status: Superseded
Proposed branch: lp://staging/~mpontillo/maas/iprange-notifications--bug-1393936
Merge into: lp://staging/~maas-committers/maas/trunk
Diff against target: 463 lines (+373/-3)
6 files modified
src/maasserver/models/config.py (+3/-1)
src/maasserver/models/signals/__init__.py (+2/-0)
src/maasserver/models/signals/iprange.py (+42/-0)
src/maasserver/models/signals/staticipaddress.py (+21/-0)
src/maasserver/models/subnet.py (+48/-0)
src/maasserver/models/tests/test_subnet.py (+257/-2)
To merge this branch: bzr merge lp://staging/~mpontillo/maas/iprange-notifications--bug-1393936
Reviewer Review Type Date Requested Status
Blake Rouse (community) Needs Information
Review via email: mp+322176@code.staging.launchpad.net

This proposal has been superseded by a proposal from 2017-04-14.

Commit message

Add notification for imminent IP address exhaustion on a subnet.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Looks good overall. The way you've done the notifications appears sound. Updating the message on a notification instead of creating a new notification will have the effect of updating the message in the UI, but will not cause the notification to pop up again for people who have already dismissed it.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Thanks for looking. Some replies below.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Didn't mean to mark this "Needs review" yet. Still need to finalize a few things and do the tests.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

This is ready for a real review now.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

Looks good for IPv4. The only thing I see missing is for IPv6? I would assume we would not show notifications for IPv6 because the number of addresses is just huge. I did not see in the code where you check if its IPv6 and not do any work. The unit tests you wrote only cover IPv4.

review: Needs Information
Revision history for this message
Mike Pontillo (mpontillo) wrote :

Well, yes and no: we should still show notifications for small IPv6 subnets sufficient to reach the threshold. I can add some tests. It should be almost the same as IPv4, but to prevent regressions it would be good to cover IPv6 as well.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

This branch is blocked on:

 - Adding unit tests for IPv6
 - Bug #1680979

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.