Opened 9 months ago

Closed 5 months ago

#5495 closed enhancement (complete)

port comment / user context to other servers (d2 and agent)

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea1.4
Component: configuration 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: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Change History (12)

comment:1 Changed 9 months ago by fdupont

Move control socket stuff to a more generic toElement function on element maps.

For d2 add comment / user context to global, ddns_domain, dns_server, tsig_key and loggers.

For ca add comment / user context to global, control socket and loggers.

Estimate time 4 hours (1 per item + 1 for review).

comment:2 Changed 9 months ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:3 Changed 8 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4

comment:4 Changed 8 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

As far as I can remember it was done. Putting under review.

comment:5 Changed 5 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:6 follow-up: Changed 5 months ago by tmark

  • Owner changed from tmark to fdupont

Compiles and unit tests pass under Ubuntu 16.04. Changes look ok, overall:

doc/examples/ddns/comments.json

Instead of using a parenthetical "i.e.":

+// It uses embedded (i.e., which will be included in configuration objects
+// and not stripped by at lexical analysis) comments.

I'd suggest something like this:

+// It uses embedded comments which will be included in configuration objects
+// within user-contexts rather than stripped away by at lexical analysis.

src/bin/agent/tests/get_config_unittest.cc

TEST_F(CtrlAgentGetCfgTest?, simple) {

This block could use some inline commentary, especially since you used
rather cryptic names "jsond" and "jsonj"

    } else {
        ElementPtr jsond;
        ASSERT_NO_THROW(jsond = parseAGENT(expected, true));
        ElementPtr jsonj;
        ASSERT_NO_THROW(jsonj = parseJSON(expected));
        EXPECT_TRUE(isEquivalent(jsond, moveComments(jsonj)));
        ConstElementPtr ca;
        ASSERT_NO_THROW(ca = jsonj->get("Control-agent"));
        ASSERT_TRUE(ca);
        pathReplacer(ca);
        EXPECT_TRUE(isEquivalent(unparsed, jsonj));
        std::string current = prettyPrint(unparsed, 0, 4);
        std::string expected2 = prettyPrint(jsonj, 0, 4);
        EXPECT_EQ(expected2, current);
        if (expected2 != current) {
            expected = current + "\n";
        }
    }

Same issue with:

src/bin/d2/tests/get_config_unittest.cc - TEST_F(D2GetConfigTest, sample1)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 5 months ago by fdupont

Replying to tmark:

src/bin/agent/tests/get_config_unittest.cc

TEST_F(CtrlAgentGetCfgTest?, simple) {

This block could use some inline commentary, especially since you used
rather cryptic names "jsond" and "jsonj"

    } else {
        ElementPtr jsond;
        ASSERT_NO_THROW(jsond = parseAGENT(expected, true));
        ElementPtr jsonj;
        ASSERT_NO_THROW(jsonj = parseJSON(expected));
        EXPECT_TRUE(isEquivalent(jsond, moveComments(jsonj)));
        ConstElementPtr ca;
        ASSERT_NO_THROW(ca = jsonj->get("Control-agent"));
        ASSERT_TRUE(ca);
        pathReplacer(ca);
        EXPECT_TRUE(isEquivalent(unparsed, jsonj));
        std::string current = prettyPrint(unparsed, 0, 4);
        std::string expected2 = prettyPrint(jsonj, 0, 4);
        EXPECT_EQ(expected2, current);
        if (expected2 != current) {
            expected = current + "\n";
        }
    }

Same issue with:

src/bin/d2/tests/get_config_unittest.cc - TEST_F(D2GetConfigTest, sample1)

and dhcp4 and dhcp6... As this is outside the ticket scope but about comments
I propose to address it in one file and to duplicate in the 3 others.

comment:8 Changed 5 months ago by fdupont

  • Owner changed from fdupont to tmark

What we do with this? BTW it needs a ChangeLog entry: "Extended user-context / comment to d2 and agent server syntax."

comment:9 in reply to: ↑ 7 Changed 5 months ago by tmark

  • Owner changed from tmark to fdupont

Replying to fdupont:

Replying to tmark:

src/bin/agent/tests/get_config_unittest.cc

TEST_F(CtrlAgentGetCfgTest?, simple) {

This block could use some inline commentary, especially since you used
rather cryptic names "jsond" and "jsonj"

    } else {
        ElementPtr jsond;
        ASSERT_NO_THROW(jsond = parseAGENT(expected, true));
        ElementPtr jsonj;
        ASSERT_NO_THROW(jsonj = parseJSON(expected));
        EXPECT_TRUE(isEquivalent(jsond, moveComments(jsonj)));
        ConstElementPtr ca;
        ASSERT_NO_THROW(ca = jsonj->get("Control-agent"));
        ASSERT_TRUE(ca);
        pathReplacer(ca);
        EXPECT_TRUE(isEquivalent(unparsed, jsonj));
        std::string current = prettyPrint(unparsed, 0, 4);
        std::string expected2 = prettyPrint(jsonj, 0, 4);
        EXPECT_EQ(expected2, current);
        if (expected2 != current) {
            expected = current + "\n";
        }
    }

Same issue with:

src/bin/d2/tests/get_config_unittest.cc - TEST_F(D2GetConfigTest, sample1)

and dhcp4 and dhcp6... As this is outside the ticket scope but about comments
I propose to address it in one file and to duplicate in the 3 others.

I'm not sure what is out of scope here. You altered these tests for this ticket, so it would be within in scope to add appropriate comments. If you wish to write it once and copy to the other locations that's fine as long as it's accurate. My issue was that the intent of the test is unclear.

comment:10 Changed 5 months ago by fdupont

  • Owner changed from fdupont to tmark

Ready for final review (I added comments to the 4 get config tests).

comment:11 Changed 5 months ago by tmark

  • Owner changed from tmark to fdupont

Changes are fine, thanks. Ready to merge.

comment:12 Changed 5 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. Closing.

Note: See TracTickets for help on using tickets.