[Deepsea-users] DeepSea 0.7.6

[Joao Eduardo Luis]

On 04/24/2017 03:20 AM, Joshua Schmid wrote:
>
>
> On 23/04/2017 15:50, Joao Eduardo Luis wrote:
>> On 04/22/2017 02:43 PM, Eric Jackson wrote:
>>> Hello everyone,
>>>   DeepSea 0.7.6 has been released.  The notable feature is the rolling
>>> upgrade.  With a running Ceph cluster, an admin can gracefully upgrade
>>> the OS,
>>> Salt and Ceph.  See my previous email about specifics.  The CHANGELOG
>>> is listed
>>> below:
>>>
>>> - Rolling upgrade
>>
>> I must say I'm a bit surprised seeing this today, just a single day
>> after you gave a "heads up about a large PR to implement the rolling
>> upgrade".
>>
>> Has this behemoth of a PR, with "81 commits and 55 files [...] Several
>> of these [... being ...] new" been properly peer-reviewed and discussed?
>>
>> I ask this because upon checking the PR on github, as well as the merge
>> commit, I see no discussion or reviews/Reviewed-by.
>>
>> It feels strange seeing such a large PR, with roughly 1.4k added lines,
>> to be announced one day and merged the very next; especially without
>> seeing any sort of involvement from anyone else (beside the authors).
>>
>
> The original commit #43 [https://github.com/SUSE/DeepSea/pull/43] has
> quite some comments and was reviewed by Blain. It has been open for a
> while now and plenty of discussions with various people have been held.
> The fashion in which the Final PR #222 was merged doesn't look as well
> reviewed, but it actually is only copy+addendum of #43/162.

I apologize for the noise then. It was not obvious such a discussion had 
been had.

In the past, I've found that in cases like these it's useful to have a 
pointer to the PR where the review process happened. If not for anything 
else, at least to allow the community around a project to have a sense 
of transparency. Not everyone follows development, but everyone likes to 
know where things are coming from when major milestones are reached.


>> Additionally, I'm inclined to presume there were no other set of eyes on
>> the PR due to commits such as
>>
>>   3dd3fe474df036ed4322b4d03da9d57934ac3baa
>>
>> which fixes a 'typo', and could have been squashed with the previous commit
>>
>>   7ca4dabbd2311a04eb39b03c2b29343970f7e476
>>
>> (which, in this case, would have reduced the number of commits in the
>> patch set).
>
> +1. I should've squashed them before.
> We haven't talked about a PR squashing habit in deepsea, yet. But I'm in
> favor of enforcing such a rule.

Call me biased, but I believe this should not require a rule to be 
enforced - it should be common sense. Squashing makes reading the 
history easier, and allows you to review individual patches. If people 
don't squash, you're forced to review the whole diff.

Besides, not squashing patches like the one I mentioned leads to 
git-bisect being completely useless, because some of the patches won't 
even compile/run/wtv.

   -Joao