Opened 2 years ago

Closed 21 months ago

#5579 closed defect (fixed)

Kea 1.3.0 premium - legal_log file name format

Reported by: benchoff Owned by: fdupont
Priority: medium Milestone: Kea1.4-final
Component: hooks Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Low
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: .5
Total Hours: .5 Internal?: no

Description

The legal_log hook library for kea-1.3.0 opens the log file named based on the date, but the day of month is not padded with leading zeros, i.e it will use basename.2018011 rather than basename.20180101. This is in rotating_file.cc RotatingFile::open().

Is there a git repo including the premium hooks? All we got was a download link.

Subtickets

Change History (14)

comment:1 Changed 2 years ago by fdupont

The bug looks easy to fix. About the question KeaPremium? in the ISC private wiki gives the answer (and all useful infos about premium hooks).

comment:2 Changed 23 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4

As discussed on 2018-03-29 call, moving to 1.4-final as low.

comment:3 Changed 22 months ago by tomek

  • Milestone changed from Kea1.4 to Kea1.4-final

As discussed on 2018-04-26 call, moving low and some med priority tickets to 1.4-final.

comment:4 Changed 21 months ago by tomek

  • Priority changed from low to medium

comment:5 Changed 21 months ago by fdupont

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

comment:6 Changed 21 months ago by fdupont

  • Add Hours to Ticket changed from 0 to .5
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to .5

Done with a test copying the (fixed) code.
Ready for review.

comment:7 Changed 21 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:8 follow-up: Changed 21 months ago by marcin

  • Owner changed from marcin to fdupont

I reviewed commit: 6adebf9a133c91cdd3159c939e0229735b2b08a5. The code change looks good but I don't quite follow an idea behind creating a test which copy pastes the fragment of the tested code. That way, when the code changes the test will keep passing, because it is not really testing the code, but itself.

Instead, I suggest creating a function in the RotatingFile class which will construct the file name from the "ymd_type" object and test that function.

Also, this change requires a ChangeLog entry.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 21 months ago by fdupont

Replying to marcin:

I reviewed commit: 6adebf9a133c91cdd3159c939e0229735b2b08a5. The code change looks good but I don't quite follow an idea behind creating a test which copy pastes the fragment of the tested code. That way, when the code changes the test will keep passing, because it is not really testing the code, but itself.

Instead, I suggest creating a function in the RotatingFile class which will construct the file name from the "ymd_type" object and test that function.

=> I had this idea too but it is a bigger change for a very minor issue (what matters is to get different names) so I gave up.

Also, this change requires a ChangeLog entry.

=> easy but what we do: add a function or merge it and postpone the function for 1.5?

comment:10 in reply to: ↑ 9 Changed 21 months ago by marcin

Replying to fdupont:

Replying to marcin:

I reviewed commit: 6adebf9a133c91cdd3159c939e0229735b2b08a5. The code change looks good but I don't quite follow an idea behind creating a test which copy pastes the fragment of the tested code. That way, when the code changes the test will keep passing, because it is not really testing the code, but itself.

Instead, I suggest creating a function in the RotatingFile class which will construct the file name from the "ymd_type" object and test that function.

=> I had this idea too but it is a bigger change for a very minor issue (what matters is to get different names) so I gave up.

I don't buy this. It is bigger change to what you have done, but it is still easy enough to do within a coffee time.

Also, this change requires a ChangeLog entry.

=> easy but what we do: add a function or merge it and postpone the function for 1.5?

I suggest doing it now and not have to get back to this.

comment:11 Changed 21 months ago by fdupont

  • Owner changed from fdupont to marcin

Done. Ready for a second round.

comment:12 Changed 21 months ago by marcin

  • Owner changed from marcin to fdupont

The code changes are fine and can be merged but I still don't see changelog for it.

comment:13 Changed 21 months ago by fdupont

Merged with Added 0 padding to the day in forensic/legal log file names. ChangeLog entry.

comment:14 Changed 21 months ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.