[#2736] Addressed review comments

Changes to be committed:
	modified:   ../../../../ChangeLog
	modified:   ../../../../doc/sphinx/arm/classify.rst
	modified:   ../parsers/client_class_def_parser.cc
	modified:   client_class_def_parser_unittest.cc

diff --git a/ChangeLog b/ChangeLog
index 29bad8f83d08e6ba1ad1af17e9a6a547ca12d863..671f881592d1bb3853116200621353d67ece046e 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,9 +1,9 @@
 2304.  [func]          tmark
-       Both kea-dhcp4 and kea-dhcp6 will now emit a warning
-       log message when classes are configured with both
-       ``only-in-additional-list`` true and parameter(s)
-       that normally impact lease lifetimes (e.g. 'valid-
-       lifetime', 'preferred-lifetime`).
+       Both kea-dhcp4 and kea-dhcp6 servers will now
+       log a warning message when they detect classes that
+       configure lease life time parameters (e.g. 'valid-lifetime',
+       'preferred-lifetime') while also setting 
+       'only-in-addditiional-list' to true.
        (Gitlab #2736)
 
 2303.  [bug]           tmark

diff --git a/doc/sphinx/arm/classify.rst b/doc/sphinx/arm/classify.rst
index 9bb8916c3214450390e3712e38420b740cd7d5d0..d6724ca9c4485f2c28459ee05b19e8e60e32b8f7 100644 (file)
--- a/doc/sphinx/arm/classify.rst
+++ b/doc/sphinx/arm/classify.rst
@@ -1170,8 +1170,8 @@ Class Priority
 ==============
 
 Client classes in Kea follow the order in which they are specified in the
-configuration (vs. alphabetical order). Additional classes are ordered by 
-pool, subnet, and then shared-network and within each scope by the order in 
+configuration (vs. alphabetical order). Additional classes are ordered by
+pool, subnet, and then shared-network and within each scope by the order in
 which they appear in ``evaluate-additional-classes``.
 
 When determining which client-class information (comprised of
@@ -1189,7 +1189,7 @@ reservation.
 
 On the other hand, lease lifetimes and DHCPv4 field values defined at class
 scope override any values defined globally, in a subnet scope, or in a
-shared-network scope.  
+shared-network scope.
 
 .. note::
    Because additional evaluation occurs after lease assignment, parameters

diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
index 3245786b6d48e98d5b32c65f4e9ec8b8ffee5288..0a2472131cc8d0af6cb073c86c446e394e3331e8 100644 (file)
--- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
+++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
@@ -297,7 +297,7 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary,
          !preferred_lft.unspecified() ||
          !offer_lft.unspecified())) {
         LOG_WARN(dhcpsrv_logger, DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES)
-                 .arg(name);
+            .arg(name);
     }
 
     // Add the client class definition

diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
index 90130b60ecf243f0b12f11e4489f6c25fb2a9bb3..86d4d4e5ed5d824e8e22ae6c1f0282ae50ffda65 100644 (file)
--- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
+++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
@@ -103,7 +103,6 @@ protected:
 };
 
 /// @brief Test fixture class for @c ClientClassDefParser.
-//class ClientClassDefParserTest : public ::testing::Test {
 class ClientClassDefParserTest : public LogContentTest {
 protected:
 
@@ -2261,11 +2260,12 @@ TEST_F(ClientClassDefParserTest, addtionalWithLifetimes4) {
         // If we expect the warning log to be emitted the occurrences
         // in the log file should bump by 1.
         if (scenario.should_log_) {
-            // Veriy we emitted another instance of the log message.
             ++exp_log_count;
-            ASSERT_EQ(countFile("DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"),
-                      exp_log_count);
         }
+
+        // Veriy we have the expected count of log messages.
+        ASSERT_EQ(countFile("DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"), 
+                            exp_log_count);
     }
 }
 
@@ -2335,11 +2335,12 @@ TEST_F(ClientClassDefParserTest, addtionalWithLifetimes6) {
         // If we expect the warning log to be emitted the occurrences
         // in the log file should bump by 1.
         if (scenario.should_log_) {
-            // Veriy we emitted another instance of the log message.
             ++exp_log_count;
-            ASSERT_EQ(countFile("DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"),
-                      exp_log_count);
         }
+
+        // Veriy we have the expected count of log messages.
+        ASSERT_EQ(countFile("DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"),
+                            exp_log_count);
     }
 }