Opened 4 weeks ago

Closed 4 weeks ago

#5396 closed enhancement (complete)

doc for shared network commands is needed (+other tweaks)

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.3-final
Component: hooks Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 7 Internal?: no

Description

The work done in #5311 added whole bunch of new commands to manage shared networks.

This ticket covers all remaining tasks related to this effort, including:

  • documentation update
  • adding more tests (including negative ones)

Subtickets

Change History (10)

comment:1 Changed 4 weeks ago by tomek

  • Add Hours to Ticket changed from 0 to 6
  • Owner set to Unassigned
  • Status changed from new to reviewing
  • Total Hours changed from 0 to 6

The code is now ready for review. Note the changes are in both kea (trac5396) and premium repos (trac5396_rebase).

Changes include:

  • new delAll() method in subnets
  • subnet{4,6}-del is now able to properly delete subnets that are parts of shared networks
  • documentation update
  • metric sh*t ton of negative tests

Please review.

Last edited 4 weeks ago by tomek (previous) (diff)

comment:2 Changed 4 weeks ago by tomek

  • Owner changed from Unassigned to tomek

comment:3 Changed 4 weeks ago by tomek

  • Owner changed from tomek to Unassigned

Ok, the tickets is back to review state. After merging #5311 to master, I rebased this code. The code in premium repo is on trc5396_rebsase branch (and trac5396 in kea repo)

comment:4 Changed 4 weeks ago by fdupont

  • Owner changed from Unassigned to fdupont

A few spelling errors so please pull.

I have a concern with last check in subnet[46]DelSharedNetwork. The comment is wrong so I fixed it and the code does the opposite of what the (fixed) comment says and is incomplete. I am building the module to understand what should and really happen(s).

Last edited 4 weeks ago by fdupont (previous) (diff)

comment:5 Changed 4 weeks ago by fdupont

  • Owner changed from fdupont to tomek

Found: comments were too cut & pasted so disturbed me. Fixed them.

So please pull and review my few changes. And after resubmit it with the doc I can't find in git diff master...trac5396_rebase.

comment:6 Changed 4 weeks ago by tomek

  • Owner changed from tomek to fdupont

I have reviewed changes and they look good. Thanks for fixing the issues. The code in main repo is on trac5396 branch, not trac5396_rebase. I only rebased premium repo, not main repo. I tried to say that in comment:3, but perhaps it was not very clear.

comment:7 Changed 4 weeks ago by fdupont

Got git diff ...origin/trac5396 on Kea so I'll resume the review...

comment:8 Changed 4 weeks ago by fdupont

Fixed a few spelling error, a typo in the doc and a typo in tests. It builds and passes checks (even before last typo fix?). IMHO ready to be merged, no need to add ChangeLog in kea or premium.

BTW I have a question: the user/admin guide says that all subnets should share the interface but it is not enforce in the code, not checked in tests for both the config and the hook...

comment:9 Changed 4 weeks ago by fdupont

  • Owner changed from fdupont to tomek

comment:10 Changed 4 weeks ago by tomek

  • Add Hours to Ticket changed from 6 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 6 to 7

Code merged. To answer Francis' question, the check for using the same interface is done in src/bin/dhcp4/json_config_parser.cc in sharedNetworkssanityChecks. I admit this applied only to loaded config files, we should probably move this to somewhere where it applies to commands.

THanks for the review.

Note: See TracTickets for help on using tickets.