Code review comment for ~paelzer/ubuntu/+source/open-iscsi:merge-2.1.2-1-HIRSUTE

Revision history for this message
Robie Basak (racb) wrote :

Not being familiar with this extensive Ubuntu delta, I particularly appreciate the use of our workflow to break the delta down with clear explanations in git commits and changelog entries. Thank you!

This looks good to me except for two possible issues I spotted.

1) I don't think 6c13643 "Don't FTBFS due to warnings new in gcc10" is needed any more with this new upstream release. My local test build without it succeeded.

2) I think 5dd176f "refresh to match 2.1.2" is broken. Before, it resulted in the line:

    iscsid.startup = /bin/systemctl start iscsid.socket

whereas now it results in:

    iscsid.startup = /bin/systemctl start iscsid.socket iscsiuio.socket

No need for a re-review after you've looked at these.

Finally, not a problem with the final result, but I thought I'd mention that I think you've drifted from the original intention of what the logical tag means, which made it harder for me to review this MP. I ended up making my own logical tag to review against instead. I would have expected 71f3193 to be squashed into 1d16906 and 41112bd squashed into 90684c2 before you tagged it logical. Otherwise the range-diff doesn't match when reviewing.

review: Needs Fixing

« Back to merge proposal