Icinga 2.14.4

[Al2Klimov]

No description provided.

[oxzi]

Thanks for creating the changelog. In general, I am very happy with it.

I added some nit-picky suggestions where I found them reasonable, as you have explicitly asked me for a review.

[oxzi]

Changelog reads good, I guess. Thanks a lot!
However, I would also wait for a review of someone more involved with the actual development in case I have missed something.

I would approve after the temporary GHA commit was removed.

[julianbrost]

All the PR references you included seem to be the PRs into the master branch, not into the support/2.14 branches as it was the case in previous releases. (And this makes it harder to compare the list of actually merged PRs with what's stated in the changelog.)

[Al2Klimov]

Especially this list (but not only) would look very monotonic then. What about giving the support branch PRs as before, but also the original ones in ( ), e.g:

Suggested change

	* Distributed Monitoring: add section "External CA/PKI". #9825
	* Explain how to enable/disable debug logging on the fly. #9981
	* Update supported OS versions and repository configuration. #10064 #10090 #10120 #10135 #10136
	* Several fixes and improvements. #9960 #10050 #10071 #10156 #10194
	* Replace broken links. #10115 #10118 #10282
	* Fix typographical and similarly trivial errors. #9953 #9967 #10056 #10116 #10152 #10153 #10204
	* Distributed Monitoring: add section "External CA/PKI". #10081 (#9825)
	* Explain how to enable/disable debug logging on the fly. #10081 (#9981)
	* Update supported OS versions and repository configuration. #10081 (#10064 #10090 #10120 #10135 #10136)
	* Several fixes and improvements. #9960 #10050 #10071 #10156 #10081 (#10194)
	* Replace broken links. #10081 (#10115 #10118 #10282)
	* Fix typographical and similarly trivial errors. #10081 (#9953 #9967 #10056 #10116 #10152 #10153 #10204)

[julianbrost]

Maybe another reason to avoid such monster backport PRs like #10081. That must have been a pain to review.

In this case, I'd just give the reference once before the list.

[Al2Klimov]

Maybe another reason to avoid such monster backport PRs like #10081.

• That was just an example! E.g Add a dedicated method for disconnecting TLS connections #10293 would be a more common example, though.

That must have been a pain to review.

Even more pain: http://al2klimov.de/linux 😎

[Al2Klimov]

In this case, I'd just give the reference once before the list.

Good idea, but doesn't apply here.

[julianbrost]

Oh wow, I wasn't aware that this problem was so widespread. This is arguable much worse than listing the PRs against master, I mean just look at that:

icinga2/CHANGELOG.md

Line 32 in b56c18b

* Several fixes and improvements of the code quality. #10124 (#10066) #10247 #10292 (#10263 #10264) #10293 (#10254)

icinga2/CHANGELOG.md

Lines 40 to 41 in b56c18b

	* Reduce the number of cluster messages in memory at the same time. #10080 (#9999) #10146 #10293 (#10210)
	* Once a cluster connection shall be closed, stop communicating. #10080 (#10213) #10295

icinga2/CHANGELOG.md

Line 48 in b56c18b

* Icinga DB, IDO: limit all timestamps to four year digits. #10298 (#10059) #10300

icinga2/CHANGELOG.md

Lines 54 to 55 in b56c18b

	* Add log messages about own network I/O. #10080 (#9993) #10237 #10294
	* Several fixes and improvements of log messages. #10080 (#9997) #10125 #10236

So for this PR, please undo these changes.

For the future, please don't combine multiple PRs in backports unless they will share a single changelog item anyway.

[Al2Klimov]

... or zero items as with GitHub actions.

[Al2Klimov]

• To my defense, I didn't create Add a dedicated method for disconnecting TLS connections #10293. Nor Runtime RPC sync failures #10292 which backported a particular PR and its code quality build deps. And that's the most sane use case IMAO. I see no problem to change-log two items for the same PR, one for X and one for code quality. That's no mess, yet. I, in contrast, would continue combining PRs in backports as long as it simplifies the backport PR review. I mean, if we list master PRs for v2.14.4, we can do that forever to avoid double standards.

[julianbrost]

I mean, if we list master PRs for v2.14.4, we can do that forever to avoid double standards.

Simply no. That's exactly why I asked to not create such backport PRs in the future. Don't use it as justification to continue doing that in the future. I'm willing to make an exception for 2.14.4 because the alternatives are just horrible. I mean who is supposed to understand what #10124 (#10066) #10247 #10292 (#10263 #10264) #10293 (#10254) as a reference list is trying to say? Yet another alternative would be to revert the backport PRs in question and do them properly, but that would just waste a lot of time now.

If you want to make an argument that providing references to the PRs against the master branch in bugfix release change logs actually makes more sense than what we're currently doing, please do so, just know that this will be a separate discussion. The mess that happened in this PR just isn't an argument for doing so.

[Al2Klimov]

And the rest of my comment? To me these are legit bundles:

• GHA – not in changelog at all
• Build deps only – changelog can reflect just the main point (or additionally mention the same backport PR once more under code quality)
  • Don't fail to remove obsolete downtimes, remove RemoveAllDowntimes() #10124
  • Runtime RPC sync failures #10292
• A bundle which, as you said, "will share a single changelog item anyway" IcingaDB: limit several numbers not to crash Go daemon #10298

But all of these of course apply only to handcrafted backports. This, in contrast, is much more interesting: https://github.com/NixOS/nixpkgs/blob/master/.github/workflows/backport.yml

[julianbrost]

Wait, if #10124 and #10292 are legit bundles, why did you list them as #10124 (#10066) and #10292 (#10263 #10264) in 6e630b5? If those were just listed as-is, it would only be half as bad. Do I really have to go over all backport PRs myself now and assess individually, how they could best be referenced in the changelog?

And the rest of my comment? To me these are legit bundles:

Yes, it can make sense, but for this release it was overused. And I'm unable to spontaneously come up with a precise rule that covers every possible situation. Please use common sense and provide a summary in the PR.

[Al2Klimov]

Do I really have to go over all backport PRs myself now

No, please wait a sec...

[Al2Klimov]

IMAO we should let is as-is now, the "over-usage" in #10080 breaks solving this issue with legitimacy.

[julianbrost]

For the record: the use of references to master PRs made this very cumbersome to review. So far, my change log review flow was to check that all PRs merged into support/* are properly assigned to the milestone and then compare the list of PRs with what's mentioned in the change log. That no longer works here.

Also, the reference to the backported PR links directly to the changes as they were done in the version to be released. In contrast, now you get to the PR into master, where you have to search the corresponding backport PR somewhere in the PR history and then end up with a huge PR which makes it really hard to keep track of things.

In summary, for previous change log PRs, it was a 5 minute task to check that the change log is consistent with what is actually in the release. Here it took ages and I'm still not sure that I didn't miss anything due to the number of indirect references I had to follow manually.