Opened 3 years ago

Closed 3 years ago

#5122 closed enhancement (complete)

SubnetParser: migrate initSubnet

Reported by: fdupont Owned by: tomek
Priority: medium Milestone: Kea 1.2 - Mozilla Milestone 1
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5037
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 9 Internal?: no

Description (last modified by tomek)

This is a follow up of #5019 and #5037. This covers migration of remaining parts of the Subnet{4,6}ConfigParser.

Subtickets

Change History (11)

comment:1 Changed 3 years ago by fdupont

  • Component changed from Unclassified to remote-management

comment:2 Changed 3 years ago by tomek

  • Description modified (diff)
  • Owner set to tomek
  • Status changed from new to assigned
  • Summary changed from migrate initSubnet to SubnetParser: migrate initSubnet

comment:3 Changed 3 years ago by tomek

  • Description modified (diff)

comment:4 Changed 3 years ago by tomek

  • Status changed from assigned to accepted

comment:5 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 6
  • Owner changed from tomek to Unassigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 6

The code is now ready for review. initSubnet migrated to SimpleParser and a lot of obsolete code removed.

comment:6 Changed 3 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:7 follow-up: Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

It should be fine to get rid of ParserContext. Now it is not critical and this can be postponed...

filed -> filled

I changed find("next-server") by get("next-server")

I deeply disagree about putting "(missing)" in a position: you must put the parent (here params) position.

Please use << in place of + when building an exception message (aka 2nd argument of isc_throw. I can fix this myself if you prefer... (2 reasons: more generic and more efficient) or we can postpone this to cleanup (as it was already in the code).

Where is the "client-class"? If it is handled at another place please leave a comment...

In ctrl_dhcp4_srv_unittest.cc it is clear that:

  • there is a missing space
  • just before a spurious second occurrence of the problem position

I note this for the cleanup ticket...

Now dhcp6/json...

If there is a reason to set the rapid commit after interface[-id] stuff (vs just after subnet6 creation) there should be a comment explaining it. If not please move setRapidCommit.

Again where is the client class?

ctrl_dhcp6_srv_unittest.cc: same issue (and likely same cause).

We should agree about the best way to deal with optional parameters: I use if contains { get ...} , you use ptr = get; if ptr ... (in fact find but I changed for get). Both are right, a good topic for the cleanup.

Another bad "[missing]" in the reservation-mode? BTW in #5123 I changed the exception raised by getString(). Cf #5121 too even IMHO we should not wait for it.

As I expected the client-class code was factorized...

Finished. I pushed one spelling and some find -> get so pull. IMHO outside the 2 "missing"s as positions there is nothing urgent.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 6 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 6 to 8

Replying to fdupont:

It should be fine to get rid of ParserContext. Now it is not critical and this can be postponed...

Yes, it is! Actually, I was very happy to remove it. I never liked it.

filed -> filled

I changed find("next-server") by get("next-server")

Thanks.

I deeply disagree about putting "(missing)" in a position: you must put the parent (here params) position.

Understood. I have updated the code as suggested.

Please use << in place of + when building an exception message (aka 2nd argument of isc_throw. I can fix this myself if you prefer... (2 reasons: more generic and more efficient) or we can postpone this to cleanup (as it was already in the code).

Ok. That code was already there, but I fixed it anyway.

Where is the "client-class"? If it is handled at another place please leave a comment...

Thanks for adding the comment. I updated it to be very specific (pointer to the actual method where client-class is now handled).

In ctrl_dhcp4_srv_unittest.cc it is clear that:

  • there is a missing space
  • just before a spurious second occurrence of the problem position

I note this for the cleanup ticket...

I fixed it in this ticket.

Now dhcp6/json...

If there is a reason to set the rapid commit after interface[-id] stuff (vs just after subnet6 creation) there should be a comment explaining it. If not please move setRapidCommit.

Not really, except we need the rapid_commit value for the logger. And we want the logger to be printed before the subnet is created in case an exception occurs. Moved setRapidCommit as early as possible and added a comment for it.

Again where is the client class?

Thanks for adding the comment. As before, I added the pointer to where the code is now.

ctrl_dhcp6_srv_unittest.cc: same issue (and likely same cause).

Fixed as well.

We should agree about the best way to deal with optional parameters: I use if contains { get ...} , you use ptr = get; if ptr ... (in fact find but I changed for get). Both are right, a good topic for the cleanup.


Another bad "[missing]" in the reservation-mode? BTW in #5123 I changed the exception raised by getString(). Cf #5121 too even IMHO we should not wait for it.

Fixed.

As I expected the client-class code was factorized...

Yes. The common parameters are moved to the common method.

Finished. I pushed one spelling and some find -> get so pull. IMHO outside the 2 "missing"s as positions there is nothing urgent.

Thanks a lot for the review. I hope these changes address all the comments. Ok, now on to review your ticket.

comment:9 in reply to: ↑ 8 Changed 3 years ago by fdupont

Replying to tomek:

ctrl_dhcp6_srv_unittest.cc: same issue (and likely same cause).

Fixed as well.

=> BTW I added in trac4501 some unit tests with a pair (v4/v6) about
subnet configuration failure. It should be nice to check this correction
(I expect the message to change a bit but to stay with one (and proper)
position).

It is a bit late to wait for a whole make (local or Jenkins) so I'll
continue tomorrow but IMHO it should work well...

comment:10 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Checked with #4501, added a space before a position and updated #4501 code.
Checked with #5123 applied (so #5122 + #4501 + #5123 order), set_config unit tests changed messages (to a better one) but it should be the only thing to do after merging of #5122 and #5123.

So ready to be merged!

comment:11 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 2 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 8 to 9

Merged! Thanks a lot for the review and your fixes. Closing ticket.

Note: See TracTickets for help on using tickets.