Code review comment for ~ahasenack/ubuntu/+source/apache2:bionic-balance-member-hostname-1750356

Christian Ehrhardt  (paelzer) wrote :

Hi,
LGTM in general.
Patches look ok as taken from upstream - it is ok that we don't change the upstream STATUS/Changelog.
Packaging Changelog seems ok to me (it would fit in three lines without too early line breaks, but that isn't important)

I agree to the magic number decision, as we would have to get all other changes up to 75 in to really announce that.

The log message before also was already clear:
  BalancerMember worker hostname (xxxxx...xxxx) too long
So I'm not sure how much that is more a feature than a backport, were there earlier versions of apache in which this worked.
But this would be for the SRU Team to decide, from the code/packaging alone it LGTM.

I also tested the steps to reproduce and can confirm the issue and the fix.

The one thing I have to nack is that maybe we an Author tag on the patches?
Usually in git-format-patch exports we have the Author listed, which is why we don't need to add it in our dep-3 tags.
But in the svn export of apache2 there is no Author - I think legally you are supposed to add something when adding them - could you find the Authors name and add it?

So you are at +0.95 just missing the Author statement in the patches I'd think.

review: Needs Fixing

« Back to merge proposal