Go-ipfs: Code Review 0.4.2..0.4.3-rc3

Created on 29 Aug 2016  路  18Comments  路  Source: ipfs/go-ipfs

I went through and code reviewed all the PRs, commits, and merges from 0.4.2..0.4.3-rc3.

First, a huge thanks to everyone. this is a huge release, with tremendous stuff. Huge shout out to everyone who worked on it, sp @whyrusleeping @Kubuxu @kevina @lgierth @RichardLitt @chriscool and all those listed here https://github.com/ipfs/go-ipfs/blob/master/CHANGELOG.md#043-rc3---2016-08-09 congrats, this is a great release with a ton of bug fixes, stability improvements, and new features. 馃帀 馃巻 馃帄 馃巿 馃帀

On this Code Review

My code review was for the purpose of finding any remaining problems before the 0.4.3 release. I read all of the code. I didn't look at everything super carefully; i focused on critical sections, UX, API changes, and security issues. I've left a bunch of comments all over the place (Merges, PRs, commits).

I took specific notes of things to address. The "MUST be addressed by 0.4.3" ones should have a commit or PR link to a _merged_ thing (edit this issue's list to add it next to the line item when you check it) before release. It's not that much, but it's also substantial. When done, please add the link like this at the end of the line: 

- [ ] <line item> -- resolved in <link-to-merged-PR>

By design, this whole list is _negative_, as the goal was identifying problems. Meaning, i only highlighted all the stuff that _needs to change_. I looked at every commit, so a lacking comment here means that those changes implicitly LGTM, and I'm fine with releasing them. So thank you for all that! 馃憤 馃槃 馃帀 馃帀

It's up to @whyrusleeping whether to do these changes on top of master or on top of 0.4.3-rc3. If we want to roll the latest changes (0.4.3-rc3..master) into 0.4.3 (instead of 0.4.4), then i will need to code review that section too. I will get started on that anyway, and should have it done by monday eve.

MUST happen before 0.4.3 release

neecommunity-input statuready
Source
picture jbenet
🎉1

Most helpful comment

@lgierth I don't know why having long RC cycles is bad thing, it only show that our QA game needs improvement but including community in throughout testing is a good thing.

Sure -- I just mean that if we know that @jbenet is still to review it, and is likely going to have lots of feedback, it's IMO not yet a release candidate. A release candidate is "almost exactly this is going to be the release, unless there are bad bugs or regressions".

picture ghost on 29 Aug 2016
👍2

All 18 comments

About rebasing PRs before merging, I can't do much in this manner, it is Jeromy who merges them, he can ping me to rebase them (he does in case of conflict) or rebase them himself (my signature won't match then).

Kubuxu picture Kubuxu on 29 Aug 2016

@jbenet thanks for all the review! The strategy will be to branch off of the 0.4.3-rc3 tag and merge everything to there. Then merge that into master once the release happens.

whyrusleeping picture whyrusleeping on 29 Aug 2016

I already created version/0.4.3-rc4 which is based off the tag and there are two PRs created to it: https://github.com/ipfs/go-ipfs/pull/3134 https://github.com/ipfs/go-ipfs/pull/3135

Kubuxu picture Kubuxu on 29 Aug 2016

Another lesson from the 0.4.3 cycle is that we should only start releasing RC builds once we're absolutely positive that this is what we want to release. We're close to a 1.5 months RC cycle.

ghost picture ghost on 29 Aug 2016
👍1

@jbenet Is blockstore.{RuntimeHashing -> HashOnRead} neccesary for 0.4.3, it would be best to keep the changeset minimal in the 0.4.3 branch to no go into merge conflict hell.

As it is small refactor (and master has already some changes with it), it would be better to keep it on master. Sounds good?

@lgierth I don't know why having long RC cycles is bad thing, it only show that our QA game needs improvement but including community in throughout testing is a good thing.

Kubuxu picture Kubuxu on 29 Aug 2016

@jbenet Is blockstore.{RuntimeHashing -> HashOnRead} neccesary for 0.4.3, it would be best to keep the changeset minimal in the 0.4.3 branch to go into merge conflict hell.

@Kubuxu it's an interface. our releases are not only binaries, they're also the code. It's pretty minimal, but okay.

jbenet picture jbenet on 29 Aug 2016

@lgierth I don't know why having long RC cycles is bad thing, it only show that our QA game needs improvement but including community in throughout testing is a good thing.

Sure -- I just mean that if we know that @jbenet is still to review it, and is likely going to have lots of feedback, it's IMO not yet a release candidate. A release candidate is "almost exactly this is going to be the release, unless there are bad bugs or regressions".

ghost picture ghost on 29 Aug 2016
👍2

Agree with @lgierth

jbenet picture jbenet on 29 Aug 2016

@jbenet (also @whyrusleeping @Kubuxu) concerning "best effort" pins, let me know what I need to do. At the moment this seams to be due to a misunderstanding, see #2698 for the reasoning behind them.

kevina picture kevina on 29 Aug 2016

Thanks, @jbenet. Good to see a lot of these comments!

RichardLitt picture RichardLitt on 29 Aug 2016

@lgierth You're generally more eloquent than I am, could you handle adding better 'error case help text' here? https://github.com/ipfs/go-ipfs/pull/2939/files

whyrusleeping picture whyrusleeping on 30 Aug 2016

@lgierth You're generally more eloquent than I am, could you handle adding better 'error case help text' here? https://github.com/ipfs/go-ipfs/pull/2939/files

Sure, here: #3158

ghost picture ghost on 31 Aug 2016

People have been checking boxes above without editing the post and adding:

- [ ] <problem description and issue links> --- resolved in <link-to-merged-PR>

see the "On this Code Review" rection

jbenet picture jbenet on 31 Aug 2016

Oh i see some RFCR things. i'll check those

jbenet picture jbenet on 31 Aug 2016

People have been checking boxes above without editing the post and adding:

sorry that was sloppy -- I clarified that section about the gateway/blocking/reverting questions. Check the linked answers, I think these are resolved. (you're right, _I_ shouldn't check them off, just provide you with enough info.)

ghost picture ghost on 1 Sep 2016

I went through and clarified referenced pull requests with ---resolved in #XXX for merged commits. I also added ---needs review #XXX for unmerged commits pending review.

Github makes it way to easy to accidentally toggle a checkbox and does not provide the history of edited comments to verify what I did. Sorry, if I did this and caused something to be wrong.

kevina picture kevina on 2 Sep 2016

@jbenet alright, your 'must haves' are all addressed at this point. For the rest, we will make issues out of each of them and get them addressed for 0.4.4

whyrusleeping picture whyrusleeping on 4 Sep 2016

Moved to 0.4.4 milestone.

Kubuxu picture Kubuxu on 12 Sep 2016
Was this page helpful?
0 / 5 - 0 ratings

Related issues

0x6431346e picture 0x6431346e  路  3Comments
daviddias picture daviddias  路  3Comments
djdv picture djdv  路  3Comments
lidel picture lidel  路  3Comments
Kubuxu picture Kubuxu  路  3Comments