Development Processes #13
Conversation
| - Updated versions of the Linux client by the package maintainers for EPEL and Debian | ||
| - Documentation about features | ||
| - Release notes / version history | ||
| - Announcements |
|
Oh - I see. Not sure why that is. I was able to comment on specific lines by doing it against the "file changes" for the overall pull request. Can you try to do it that way? (and wouldn't you want to do it that way rather than against a specific commit that might get updated later?) |
|
@davidpanderson wrote the following on boinc-dev. I'm quoting him here in order to keep the conversation in this thread.
|
The difference is the use of cherry picking vs creating feature branches off of the release branch and then merging the release branch making the changes against the release branch. The advantage of cherry-picking is that the developer workflow never changes (i.e. if they want to fix something, they just branch from master, make the fix, submit a pull request and it gets merged back in) which reduces the coordination required between the release manager and the rest of the community. The disadvantage is that cherry-picking has the potential for bringing in unrelated and partial changes from other code which can create unexpected bugs and issues. In the earlier stages of the 7.10 release, it was relatively easy to cherry-pick. However, as the release branch and the master branch diverged the cherry-picks started to bring in other code. If we keep the test and release cycle short (2-3 weeks) then I personally don't have a strong opinion about either approach.
I think it is wise to have a standard way of doing things and we should work to do things in that standard way. It helps the community operate together if there are standard expectations of how to do things. If something is "completely up to them" on how to do things, this prevents others from understanding how best to prepare their work to be usable by the release manager. The standard policy could be "The release managers will cherry-pick those fixes they want to include in the release from the master branch". That then informs the contributors how they should implement their code so that it is available for the release manager to use. I also think that a release branch should be treated like the master branch in that any changes to the branch should be done via a pull request process where another person reviews the pull request before it is merged (the two pairs of eyes principle). |
|
|
||
| The release branch is considered feature frozen. Only bug fixes designated by the release manager will be merged into the release branch. | ||
|
|
||
| Bug fixes designated by the release manager for fixing within the release should be branched from the release branch and merged directly into the release branch. Doing this reduces the chance of inadvertently bringing in unrelated code changes. |
There was a problem hiding this comment.
I strongly agree with this because of the inherent dangers in cherry-picks from master. To make this statement complete, though, you need to add that the release branch needs to be merged into master after the bugfix PR got merged into the release branch, thereby fixing the bug in master as well.
|
|
||
| Bug fixes designated by the release manager for fixing within the release should be branched from the release branch and merged directly into the release branch. Doing this reduces the chance of inadvertently bringing in unrelated code changes. | ||
|
|
||
| If the release manager determines that some code should be merged from master into the release branch, then this can be done. However, this should be viewed as an unusual step and carefully considered due to the risk of introducing unexpected change. |
There was a problem hiding this comment.
We need to be careful with the terms used to avoid confusion: the notion of "some code" doesn't make (a lot of) sense in the context of a "merge". A merge always comprises all code leading up to the commit you merge.
I couldn't agree more. That's why there are process models... |
Of course! |
Whether something make sense or not depends on what the baseline is.
That's a statement that describes how things have been done. What we're trying to do here is to agree on how things should be done, improving on what we've improved already. We already introduced a number of best practices and it's good to see that the whole development velocity has increased significantly, while increasing stability at the same time. We should build upon that! IHMO GitFlow could really help us settle the few remaining questions in how to handle code changes, which is why I urge everybody to have a look at it again - now with more git experience under everyone's belt, gathered over the past months. We're almost using that model anyway (feature branches, develop/intergration branch, release branches, tagging) and it specifically addresses the question of how hotfixes are meant to be integrated. Yet formally agreeing to use it would simplify things a lot, it's already well documented in various places and even adds tool support that, I reckon, is vital for a number of contributors. |
|
I'm not a huge fan of gitflow largely because of the creation of the develop branch and the not very useful use of the master branch (http://endoflineblog.com/gitflow-considered-harmful). I think that Github Flow + Release branches is easy to understand and gives us all the flexability that GitFlow provides. (Note that Github flow + release branches is basically the so called Gitlab Flow) I think that there is a massive value in being able to tell contributors to BOINC - want to fix a bug or add a feature? Then create a branch from master, fix the bug in the branch and create the pull request to merge back into master. This allows most developers to have a very simple mental model of how to contribute and the defaults of git work directly with that model with a low learning curve required. |
|
To be honest, I don't care about naming, I care about structure and workflow. That is, I don't want or need an exact copy of the GitFlow model. Their "develop" branch resembles more or less our "master" branch, so that can stay as is. But coming back to what I highlighted above: our current way of handling hot fixes is flawed and I think that needs fixing. If fix branches are branched off and merged back into "master" you'll get yourself into trouble applying those fixes to your release branch - if cherry-picks are a no-no, which they are for this purpose. I think you and I agree on that. So we need to find a way how to handle those. Branching off and merging back into the release branch would be fine as long as you also merge the release branch into "master" afterwards. But what do you do with older release branches? Are those still supported? How do they get the fix without cherry-picking or feature-creep? I only see two option to solve this right now:
Ideas? |
|
In the past, our policy, or at least our goal, has been that it should always be possible to create a bug-free build from master. In other words, nothing should be changed in master until it has been thoroughly tested for functionality, not just that there are no compiler or linker errors. Unfortunately, that was not always followed in practice. To accomplish that, all changes should initially be made in a release branch, and the release branch should be merged into master only after alpha testing. It had been my understanding that was the plan starting with 7.12. The approach proposed here is a 180-degree shift from my earlier understanding, and it means that only a previous release branch will meet those criteria. I'm not saying this is necessarily a problem. I just want to point out that this proposal is a significant policy shift. |
That's still the case today and should be in the future.
As long as you mean "fixes" when you say "changes" I do agree (well, in branches off a release branch to be precise). Changes as in "new features" are certainly not developed off "a" release branch (which "a" anyway?). That's what feature branches are for and those are branched off of master.
What do you mean by "the approach"? We discussed various ideas in this discussion. Also, if you really see a 180-degree shift (rather a rotation) then there seems to be a misunderstanding somewhere. This could be related to your understanding that any changes are "made in a release branch". So we need to clarify that first and get rid of a potential confusion. |
Sorry that I was not as clear as I should have been. My comments refer to the proposed client release process in its current form. It specifies that feature branches will be merged into master, and that the release branch won't be created until
Since alpha testing does not occur until the installers are built from the release branch, this implies that untested code will be merged into master, which is incompatible with keeping master bug-free. |
|
Ah, thanks for the clarification. I let Kevin give the "authoritative" answer to this but here's my take: There's a difference between developer testing, integration testing and alpha/beta testing. The first happens while a developer works on a new feature locally. Integration testing is what ideally happens as part of the review process. That is, PRs are being tested in the personal test environment of the reviewer and only merged (into master) when they passed review. That ensures a rather stable master branch and that's what we should aim for. Subsequent alpha/beta tests of upcoming release are meant to find the few remaining bug that only show themselves in much wider test that are infeasible to do for every single PR. Also, "bug-free" is a state you'll never reach, not even via alpha/beta testing - otherwise you wouldn't have to maintain a release once it's out. You instead aim for something that's tested up to a level where you're confident to release it (use the 80/20 rule). |
|
I'm confused about the statements about previous policy. When we started drafting the "Development Workflow" and the "Client Release Process", @davidpanderson wrote this up to describe the process used: https://boinc.berkeley.edu/trac/wiki/AdminReleaseManagement
The proposed process between the "Development Workflow" and the "Client Release Process" should be the following (I suspect that a document dedicated only to branch and merge strategy across both documents might be useful - I'll see what I can do unless someone else wants to create it).
My understanding that the most significant difference between the old process (which I believe was used for 7.10) and the new process the last two bullet points. I have been theoretically opposed to cherry-picks in the past due to their potential for issues. However, in practice during the 7.10 release I had no issues with actually using them so I'm open either going with the proposal above or making it that ALL development is done as feature branches off of master and that fixes for bugs found during release testing are simply cherry-picked to the release branch. The advantage of doing cherry-picks is developers always know that the simply branch from master and create the pull request to merge back into master and there development patterns never change. That would reduce the coordination required between people which is a good thing in this environment. |
|
For the branch/merge policy - I would like to propose the following principles:
Any other requirements we would need? I put forward the following as a starting point to discuss how to meet these requirements. Note that this is a straw man for people to refine and discuss.
How can this be improved to help support translations? Do we need a translation branch so that we can merge from master -> translation so that we can code freeze and wait for the translations and then go from translation -> release branch? I don't quite understand the needs of the transfix. If someone could help her that would be good. (@ChristianBeer ?) |
|
If the consensus really is to increase the use of cherry-picks then can we please agree to cherry-pick only merge-commits and no standard commits whatsoever? This way we could at least ensure that a fix, contributed via a PR, doesn't lose coherence (compared to trying to cherry-pick its individual commits). This also means that PR must never be merged as a fast-forward merge, only as a merge-commit. |
|
That is a good point and one I agree with. The cherry-picks that I did were all the merge commits. If you read https://help.github.com/articles/merging-a-pull-request/ it says that clicking the merge button on github automatically uses the --no-ff option so that works to keep it easy since that will be the primary way that people do merges. So updating the proposal would be:
|
|
Yes, I was thinking about the command line, not GitHub. Thanks for adding that. Regarding "feature branches": this might be confusing since when you add that specifier then contributors might expect "fix branches" somewhere as well. These two terms are often use together and the term "feature branches" are typically reserved for new features only, not fixes. Thus I would just omit that specifier. It doesn't make the statements any less clear I think. |
|
Updated proposal:
|
|
Deal 👍 |
|
I'd like it if other people could chime in on this as well. Definitely @CharlieFenton since he has already been participating in the discussion but also other committers and contributors would be good. |
|
As I've pointed out repeatedly, the client in master cannot be guaranteed
to be bug-free.
"Bug-free" means it works on an exponentially huge space of computer HW and
SW,
configuration settings, use cases, sets of projects, etc.
The PR review process tests one point in this space (and doesn't test it
thoroughly).
Alpha test is our only way of sampling the rest of the space.
…On Fri, Jun 1, 2018 at 9:45 AM, Kevin Reed ***@***.***> wrote:
I'd like it if other people could chime in on this as well. Definitely
@CharlieFenton <https://github.com/CharlieFenton> since he has already
been participating in the discussion buy other committers and contributors
would be good.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA8KgWUkXvhTP8YUM6k9QD8z70xSZ0g4ks5t4W-agaJpZM4UJM-y>
.
|
|
I believe that master can be high quality as long as:
However, I do believe that release branches are required in order to have that concentrated effort from multiple people that alpha testing provides in order to cover more of the complex test space that is not covered via the tests and reviews done for pull requests. The branch/merge proposed above supports that and once I hear back from a few more people, I will update the client release process document to reflect that (I might actually add a document that explains the branch/merge strategy on its own and update the client release and development work flow documents to refer to it). |
|
The review process means that a particular new feature works on 2
particular computers.
If that's what you mean by "high quality", OK.
…On Fri, Jun 1, 2018 at 2:14 PM, Kevin Reed ***@***.***> wrote:
I believe that master can be high quality as long as:
- Developers test their new functionality well before opening a pull
request
- The review process here
<https://github.com/BOINC/boinc-policy/blob/master/Development_Documents/Development_Workflow.md#21-code-review>
is done by a second person for each pull requests
- Our automated tests are run against the pull request (and over time
as they get more sophisticated this testing will correspondingly be better)
However, I do believe that release branches are required in order to have
that concentrated effort that alpha testing allows by multiple people in
order to cover more of the complex test space that is not covered via the
tests and reviews done for pull requests.
The branch/merge proposed above supports that and once I hear back from a
few more people, I will update the client release process document to
reflect that (I might actually add a document that explains the
branch/merge strategy on its own and update the client release and
development work flow documents to refer to it).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA8KgTeXloCiZxmUQm5iUR-QqnPrZTYgks5t4a6hgaJpZM4UJM-y>
.
|
|
Good catches - I've fixed. Thanks! |
Development_Documents/BOINC_Flow.md
Outdated
| A release branch is created off of the master branch. Since the release branch is considered feature frozen, new code should only be added to the branch if the release manager determines that it is necessary. | ||
|
|
||
| Any code that is added to a release branch after it is created should be a cherry-pick of a merge commit that is present on the master branch. The merge commit would be the result of a merging a bugfix or feature branch that was created and merged as normal. | ||
|
|
There was a problem hiding this comment.
Cherry-picking a (random) commit from another branch (here: master) might not be the best possible solution. There is no (technically supported) way to ensure that
- this single commit that works on one branch doesn't break the (build of) the release branch and
- everything that is needed to fix the problem is contained in the single commit
For creating a bugfix branch I would recommend to fork from the last common commit of the master and (last) release branch, and then separately merge into master and release branch via PRs.
There was a problem hiding this comment.
So it isn't cherry-picking a random commit but rather cherry-picking a merge commit. The community has been doing pretty well with making their feature branches atomic so the chance of bringing in misc code is significantly reduced (not eliminated).
If you can provide any easy way (one line command) to identify the commit where a branch was forked, then I would be happy to add diagrams and instructions stating that if you are making fix intended for a release branch then you should create your branch from that commit.
|
I don't have anything to add, other than the general policy outlined here looks good. I defer to those who are more knowledgeable about software engineering to determine the exact details. As long as it's not |
…ranch should be branched from the commit the release branch forked from.
|
I've updated the "BOINC Flow" documentation to reflect the comments of @bema-ligo and @brevilo. I would like anyone interested to comment. If there are no further comments we will shift to voting to approve in the next week or two. |
|
This is an official call for a vote of the committers to approve this pull request as announced on boinc_dev. The BOINC governance document states that decisions regarding the software development process are voted on by the committers using the process described as "majority voting". This means that 2/3rds of the committers will need to cast a vote and of those that vote a majority must vote to approve the item. The process calls for a 14-day discussion period that starts when this email is sent. At the end of the 14-day discussion period, there will then be a 7 day voting period. At the conclusion of the 7 days the votes will be counted. Although community members who are not committers cannot vote in this process, they are encouraged to voice their opinion in favor or against this change. @davidpanderson @ChristianBeer @CharlieFenton @lfield @SETIguy @AenBleidd @marius311 @tristanolive @adamradocz @Rytiss @JuhaSointusalo @romw - You are all committers and need to review and vote on this pull request. Thanks! Please contact me with any questions. |
|
@TheAspens: I am comfortable with this in its present form. Please remind us when the 14-day discussion period ends and the 7-day voting period begins. |
|
@CharlieFenton - thanks for the comment. I will tag everyone again (and email boinc-dev) when the 7 day voting starts. |
|
The 14 day discussion period has elapsed and the voting period is now open. Can all committers please vote to either approve or reject the changes in this pull request. The committers are: @davidpanderson @ChristianBeer @CharlieFenton @lfield @SETIguy @AenBleidd @marius311 @tristanolive @adamradocz @Rytiss @JuhaSointusalo @romw, @brevilo and @Uplinger. There are 15 committers so we need 10 of them to vote in order for this vote to be valid. Thanks! |
|
I vote to approve the changes in this pull request. |
|
I read through the whole set of updates once again and still have a number of comments and questions. Since I missed to have those discussed during the official period I'll now abstain from voting (assuming that's possible) and raise my concerns afterwards. |
|
Voting to approve the changes. |
|
I vote to approve the changes.
Best regards,
Vitalii Koshura
пн, 27 авг. 2018 г. в 17:22, Rytis Slatkevičius <notifications@github.com>:
… Voting to approve the changes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADFZoT7gErytcLbJeiD2wSElVNNWl7Q2ks5uVACegaJpZM4UJM-y>
.
|
|
I vote to approve these changes. Thanks, |
|
I vote to approve the changes.
…On Mon, Aug 27, 2018 at 8:01 AM, Uplinger ***@***.***> wrote:
I vote to approve these changes.
Thanks,
-Keith Uplinger
World Community Grid
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKXcsrtq9nKwfRjkJj2olO8I1p-s2KTVks5uVAnRgaJpZM4UJM-y>
.
--
Eric Korpela
korpela@ssl.berkeley.edu
AST:7731^29u18e3
|
|
I vote to approve the changes in this pull request. |
|
I vote to approve the changes. |
1 similar comment
|
I vote to approve the changes. |
|
I vote to approve the changes |
|
I vote to approve the changes. @brevilo Further changes can still be made via a separate pull request. |
That was my idea as I don't want to block this one from being merged right now. |
|
I vote to approve. Thanks for the work on this, I'd say this is the flow that I, a not-super-experienced developer, am most familiar with, and so this bodes well for making it easy to onboard future more experienced developers, who I imagine will be at least familiar with this style. |
|
The voting is complete. It passes with a vote of 12-0 with 3 people not voting. Voting Yes: @ChristianBeer @CharlieFenton @lfield @SETIguy @AenBleidd @marius311 @tristanolive @adamradocz @Rytiss @JuhaSointusalo, @TheAspens and @Uplinger. Not Voting: @davidpanderson, @romw, @brevilo |
This is a proposal from the working committee on how to manage client releases. It also includes updates to the development workflow and a document that describes the proposed branch/merge strategy for BOINC.