Git workflow proposal #3
Conversation
daviddavis
changed the title
pup-0003.md
Outdated
| Another minor caveat is that changes that we want to only be made to older | ||
| release branches (but not to newer ones) still need to be merged forward. We | ||
| could elminate the need for this with cherry picking. It's worth noting that | ||
| this rarely happens. |
There was a problem hiding this comment.
How often does this happen? Is it worth mentioning?
daviddavis
force-pushed
the
pup3
branch
5 times, most recently
from
031a25e
to
d4f11c7
Compare
pup-0003.md
Outdated
| Switching to this model also creates some inconsistency. For example, if we | ||
| adopt this for Pulp 3, then we still have to use merge forwarding for Pulp 2. | ||
| Then we are stuck having to support two different strategies. | ||
|
|
There was a problem hiding this comment.
Anything else I missed here?
There was a problem hiding this comment.
I think this approach makes it more likely that a fix will be written utilizing code that is new in master, and not present on the current release branch. This requires extra scrutiny and awareness on the part of the person cherry-picking a change back, to ensure that all code touched or used by a change exists on the release branch and behaves the same. Sometimes it will require cherry-picking intermediate changes that might not otherwise be obvious.
Of course the risk can be mitigated with good unit tests, but that also requires writing good unit tests. ;)
There was a problem hiding this comment.
Defaulting to master, and requiring extra steps to get a change onto the current release branch, is likely to result in fewer bug fixes making it onto current release branches. We can and should try to be diligent about cherry-picking fixes to the release branch, but the path of least resistance may prove too tempting at times.
There was a problem hiding this comment.
Updated to include both drawbacks.
daviddavis
force-pushed
the
pup3
branch
2 times, most recently
from
7ff67f5
to
80431ef
Compare
pup-0003.md
Outdated
|
|
||
| This PUP is to use cherry-picking instead of merging forward. Using this model, | ||
| all PRs would be made against master and then changes would be cherry-picked to | ||
| release branches as needed. |
There was a problem hiding this comment.
I wonder if we need to elaborate on the "as needed" part. As noted in one of my comments below, this approach could temp us to put fewer fixes onto the current release branch.
Right now all bug fixes default to landing on the current release branch and master; does this PUP propose changing that default? Either way, I think it's worth stating explicitly.
There was a problem hiding this comment.
Right now all bug fixes default to landing on the current release branch and master; does this PUP propose changing that default?
I don't think so. I will add it though. What's the policy around hotfixes and release branches?
There was a problem hiding this comment.
There is some info about hotfix and release branches here.
My understanding is that right now if hotfix is needed, we create a separate branch from the <current>-release branch, cherry-pick what is needed and when hotfix is released its branch will be merged forward to <current>-release, then to <current>-dev, then to master. The only special case is when <current>-dev contains only the fixes needed for hotfix, hotfix release can be build from this branch then.
There was a problem hiding this comment.
@goosemania you say <current>-release but could it also be an older release? Like if we found say a security issue in 2.9, we would cherry-pick the change to 2.9-release, right? Then would we cherry-pick the change to 2.10-release, and so forth?
There was a problem hiding this comment.
I added some text to point readers to the Design portion which I've updated to specify which items need to be cherry-picked where. Let me know if that doesn't suffice.
pup-0003.md
Outdated
| and then to master (a process called "merging forward"). More info about | ||
| merging forward can be found in our documentation: | ||
|
|
||
| http://docs.pulpproject.org/en/nightly/dev-guide/contributing/merging.html#merging-your-changes |
There was a problem hiding this comment.
I think referring to nightly docs in PUP is not safe for the history, it can be changed if this or similar PUP will be accepted ;)
Here is a link to 2.12 (assuming that when we will update merging docs it will happen in master branch): http://docs.pulpproject.org/en/2.12/dev-guide/contributing/merging.html#merging-your-changes
That's another story but now I am thinking maybe it would be good to have certain docs sections (like "contributing") not to be versioned together with pulp releases...
pup-0003.md
Outdated
| --- | ||
| title: Cherry-Picking Commits | ||
| author: David Davis | ||
| created: 01-Apr-2017 |
There was a problem hiding this comment.
I like this date, nobody knows if this PUP is a joke or not ;)
pup-0003.md
Outdated
| first merge the commit to master and then cherry-pick the commit to any | ||
| release branches that required the change. For a bugfix, this would be the | ||
| latest release branch. For hotfixes, this would be the oldest affected | ||
| release branch plus any newer branches. |
There was a problem hiding this comment.
It may be obvious but do you think it is worth writing some glossary/explanation what branches do we have now and what does the current release branch mean, for example.
Currently we have:
- X.Y-release branches
- X.Y-dev branches
- master branch
Because of the branch names we have, there could be a confusion. When the term "current/latest release branch" is used at least my first thought was why X.Y-release branch and not X.Y-dev, though I think you and everyone meant -dev. Maybe it is just me but for historical reasons when everything will be different and someone will review this PUP it would be easier to understand what we meant, if we write down such obvious things.
There was a problem hiding this comment.
Good idea. I added a link to the branching doc of 2.12. I think that should provide a historical record of our branching scheme?
pup-0003.md
Outdated
| the commit, it retains the same id. It makes looking up the commit to see what | ||
| branches it's been applied to easier. If we cherry-pick, we have to track this | ||
| information externally to git. This means also having to remember to update | ||
| redmine. |
There was a problem hiding this comment.
Maybe something like git log --all --grep='#<issue_number>' --oneline --decorate will help to identify branches which contain certain fix. Though not all commits contain redmine issue.
daviddavis
force-pushed
the
pup3
branch
2 times, most recently
from
3c13e08
to
475f19d
Compare
pup-0003.md
Outdated
| title: Cherry-Picking Commits | ||
| author: David Davis | ||
| created: 01-Apr-2017 | ||
| status: |
There was a problem hiding this comment.
It is not overly presumptuous to add "Accepted" here. Making the change before merging is an extra step that is likely to be forgotten.
There was a problem hiding this comment.
+1 to this. Since it lives as a PR for a long time I think putting 'Accepted' here will set the content up ready to be merged. It's up to you though @daviddavis
pup-0003.md
Outdated
| Currently in Pulp, we merge features to master. However, hotfixes and bug fixes | ||
| are merged to release branches which then are merged to newer release branches | ||
| and then to master (a process called "merging forward"). More info about | ||
| merging forward can be found in our documentation: |
There was a problem hiding this comment.
This is a good summary of the entire PUP, but IMO this section should only be a simple outline of the proposed workflow. After the PUP is ratified, it will make more sense to have a description of today's system in the Motivation section than the summary.
pup-0003.md
Outdated
| @@ -0,0 +1,154 @@ | |||
| --- | |||
| title: Cherry-Picking Commits | |||
There was a problem hiding this comment.
This PUP is more general than defining our cherry-picking policy. It is redefining our Git workflow, so I would prefer the title: Git-Workflow.
pup-0003.md
Outdated
| This PUP is to use cherry-picking instead of merging forward. Using this model, | ||
| PRs would be made against master and then changes would be cherry-picked to | ||
| release branches as needed. For a more detailed workflow, see the Design | ||
| section. |
There was a problem hiding this comment.
"For a more detailed workflow, see the Design section." is not really necessary.
pup-0003.md
Outdated
| There are a few motivating factors for this proposal. First, our git history is | ||
| complex with multiple branches feeding into one another and numerous merge | ||
| commits. Using cherry-picks would simplify our git history by keeping it more | ||
| linear and eliminating merge commits. |
There was a problem hiding this comment.
This section is clearer if each reason is the first part of its paragraph, I don't think the transitions are necessary.
Our git history is complex....
Opening a PR requires knowledge....
It is difficult to make a change in an old branch without bringing it forward...
This system will be more resistant to merge error....
pup-0003.md
Outdated
| ## Detailed Design | ||
|
|
||
| This proposal would keep the current branching scheme [1] involving master, | ||
| x.y-dev branches, and x.y-release branhces. The workflow by which changes are |
pup-0003.md
Outdated
| x.y-dev branches, and x.y-release branhces. The workflow by which changes are | ||
| moved between the branches would however change. | ||
|
|
||
| ### Story |
pup-0003.md
Outdated
|
|
||
| ### Bug Fix | ||
|
|
||
| 1. All bug fix PRs are also opened against master |
There was a problem hiding this comment.
s/also// I would prefer if the process for each is described without needing to reference each other.
pup-0003.md
Outdated
|
|
||
| ### Hotfix | ||
|
|
||
| The current hotfix process [2] would remain basically unchanged. However, |
There was a problem hiding this comment.
Same as above. I'd like the hotfix process described here.
There was a problem hiding this comment.
I tried to do that with the steps below. Did I leave something out?
There was a problem hiding this comment.
Oh, you mean the current hotfix process? I can do that.
There was a problem hiding this comment.
I thought about it some more and since they are very similar (old process vs new process), I'm hesitant to do so since it would be redundant. I think linking to it should suffice but let me know if you disagree.
There was a problem hiding this comment.
+1 to linking to avoid repeating yourself.
pup-0003.md
Outdated
| 4. The person cherry-picking would also need to track in redmine which release | ||
| branches the change has been made to. | ||
|
|
||
| ### Updating redmine |
There was a problem hiding this comment.
The specifics of this are important to figure out, but I think it might be helpful to decouple our git strategy from our external tracker.
There was a problem hiding this comment.
We could do that but we probably need a process in place to track where cherry-pick commits end up before we change our git workflow.
There was a problem hiding this comment.
That sounds reasonable to me. Assuming that the process for tracking cherry-picked commits needs to be a part of this decision, one of these proposals should be described in detail and the others should be moved to Alternatives. If there is not a consensus yet, then let's prod pulp-dev mailing list to reach one.
pup-0003.md
Outdated
| * If we adopt this proposal, how do we track in redmine what release branches a | ||
| fix has been applied to? | ||
| * How do we transition to this model? Should we use it only for Pulp 3 for | ||
| instance? |
There was a problem hiding this comment.
the idea of using 2 git workflows does not inspire me much. Previously we've mentioned that among drawbacks of current approach, community users do not know against which branch to open a pr. I with with adoption and usage of 2 workflows would blow their mind completely.
There was a problem hiding this comment.
+1 to doing it for both Pulp2 and Pulp3
pup-0003.md
Outdated
| ## Summary | ||
|
|
||
| Currently in Pulp, we merge features to master. However, hotfixes and bug fixes | ||
| are merged to release branches which then are merged to newer release branches |
There was a problem hiding this comment.
A few accuracy things about this section/line.
- s/release/devel/
- Only one merge forward occurs but this section says two. The bug fix is merged to the devel branch which are then merged to master.
- hotfixes are pretty special so perhaps disinclude them.
pup-0003.md
Outdated
| http://nvie.com/posts/a-successful-git-branching-model/ | ||
|
|
||
| This PUP is to use cherry-picking instead of merging forward. Using this model, | ||
| PRs would be made against master and then changes would be cherry-picked to |
There was a problem hiding this comment.
s/changes/bugfixes/ <--- because we only would cherry pick back a bugfix, never a feature
pup-0003.md
Outdated
|
|
||
| This PUP is to use cherry-picking instead of merging forward. Using this model, | ||
| PRs would be made against master and then changes would be cherry-picked to | ||
| release branches as needed. For a more detailed workflow, see the Design |
There was a problem hiding this comment.
s/release branches/the devel branch/
There was a problem hiding this comment.
I leave ^ because there is only one branch and it's a devel branch.
pup-0003.md
Outdated
| could elminate the need for this with cherry-picking. It's worth noting that | ||
| this rarely happens. | ||
|
|
||
| Lastly, we've also seen some mistakes where branches are accidentally merged |
pup-0003.md
Outdated
| this rarely happens. | ||
|
|
||
| Lastly, we've also seen some mistakes where branches are accidentally merged | ||
| into other branches. Commits could be accidentally cherry-picked but this would |
There was a problem hiding this comment.
Some extra sentences prior to the sentence starting with 'Commits'.
These situations have occurred multiple times, and each time required multiple people several hours to resolve. After being resolved the git history is convoluted with huge negative commits which later caused confusion at release time.
pup-0003.md
Outdated
| all PRs are opened against master and the onus to update release branches is on | ||
| the person merging (who usually has better knowledge of which branches a commit | ||
| needs to be applied to). | ||
|
|
There was a problem hiding this comment.
Here's a distinct point to add as a new paragraph:
When the merge forward happens, sometimes the implementation needs to be reworked and conflict resolution done. This reworking never goes through a review process and is usually done by a single developer who tries to get it right. This has caused issues in the past when a merge forward left the implementation on master broken.
There was a problem hiding this comment.
So we could still have the reverse problem though—cherry picks to devel branch could be break the devel branch. This process doesn't necessarily address this problem unless we require cherry-picks to be reviewed.
There was a problem hiding this comment.
I don't think a second review will practically find many issues. If there is an issue I think it will only be found through usage and testing. I'm pitching a process whereby if it applies cleanly it's picked and if not then it's skipped mainly for efficiency reasons.
pup-0003.md
Outdated
| release branches (but not to newer ones) still need to be merged forward. We | ||
| could elminate the need for this with cherry-picking. It's worth noting that | ||
| this rarely happens. | ||
|
|
There was a problem hiding this comment.
Here's a distinct point to add as a new paragraph:
When merging forward a pull request from a community PR the core developer must assume the responsibility to rework the contribution from the community member in cases where the implementation changes. This causes a simple merging responsibility to turn into a development responsibility which the developer merging likely did not plan to do. By accepting all pull requests exclusively to master the core developer never unexpectedly needs to take ownership of an implementation.
There was a problem hiding this comment.
The same could be true of the cherry-pick though: cherry-picking a bug fix to the devel branch could require rework.
There was a problem hiding this comment.
I hope we don't do the rework: https://www.redhat.com/archives/pulp-dev/2017-April/msg00050.html
pup-0003.md
Outdated
| 1. All bug fix PRs are also opened against master | ||
| 2. After the PR is ready to be merged, the person merging the commit would | ||
| first merge the commit to master. Then, this person would cherry-pick the | ||
| change from master to the latest x.y-dev branch. |
There was a problem hiding this comment.
I like step (2) if the cherry pick applies cleanly. If the cherry pick does not apply cleanly I think leaving it on master is fine for most situations.
There was a problem hiding this comment.
Aren't these bug fixes that would need to go to the devel branch?
There was a problem hiding this comment.
Bugfixes should go to the current bugfix release stream, but if it needs conflict resolution or reworking of the implementation then it's probably not worth it unless the bug is very important. What qualifies as very important is subjective, but as an absolute policy I don't think we need 100% of bugs to land on the bugfix stream. I think that time could be better spent fixing more bugs versus fixing the same bug twice so users can receive the fix a few weeks earlier.
There was a problem hiding this comment.
Oh I wrote some about this here: https://www.redhat.com/archives/pulp-dev/2017-April/msg00050.html
pup-0003.md
Outdated
| 1. The affected release would be branched and a hotfix would be applied. | ||
| 2. After the release, the branch gets merged into the x.y-release branch. | ||
| 3. The change is then cherry-picked into the x.y-dev branch and then | ||
| cherry-picked forward to any other x.y-dev branches and master. |
There was a problem hiding this comment.
There should be exactly 1 branch to cherry pick forward to and it should be master.
There was a problem hiding this comment.
What if we find a security issue in say pulp 2.8+. Would we not create have to cherry-pick against 2.8 and then cherry-pick the hotfix to 2.9, 2.10 etc?
There was a problem hiding this comment.
My understanding of the current process is that we would only release against the current release stream ever. It is important for users to stay up to date. As Pulp is packaged in various distros those packagers could choose to apply the security fix onto their older, packaged version of Pulp.
pup-0003.md
Outdated
| merging commits forward from devel branches. Using this model, PRs would be | ||
| made against master and then bug fixes would be cherry-picked to the devel | ||
| branch as needed. Hotfixes would be cherry-picked from release branches into | ||
| devel branches and master. |
pup-0003.md
Outdated
|
|
||
| #### New Feature | ||
|
|
||
| 1. All feature PRs are opened and merged against master. |
There was a problem hiding this comment.
Maybe add a second sentence here to clarify. No cherry picking will occur since new features only land on master.
pup-0003.md
Outdated
| The current hotfix process[2] would remain basically unchanged. However, | ||
| instead of merging forward the commit, we'd use cherry-picking. | ||
|
|
||
| 1. The affected release would be branched and a hotfix would be applied. |
There was a problem hiding this comment.
Maybe for clarity? s/branched/branched to create the hotfix branch/ s/applied/applied to the hotfix branch/
pup-0003.md
Outdated
|
|
||
| ### Conflicts | ||
|
|
||
| If the cherry-pick has conflicts, we should consider abandoning the cherry-pick |
There was a problem hiding this comment.
I was hoping for stronger language here, but maybe others feel differently. Stronger would be s/consider abandoning/abandon/.
pup-0003.md
Outdated
|
|
||
| ### Using git cherry-pick | ||
|
|
||
| When we perform the cherry-pick in git, we should use the `-x` flag which adds |
pup-0003.md
Outdated
|
|
||
| ### Updating Redmine | ||
|
|
||
| When a cherry-pick is pushed to a dev branch, the release field of the issue in |
There was a problem hiding this comment.
s/pushed/merged and pushed/ or s/pushed/merged/ maybe?
pup-0003.md
Outdated
| ### Updating Redmine | ||
|
|
||
| When a cherry-pick is pushed to a dev branch, the release field of the issue in | ||
| Redmine should be set to x.y.next. For example, when a cherry-pick is pushed to |
There was a problem hiding this comment.
similar statement here about pushed as in the comment above on L#95.
pup-0003.md
Outdated
| When this proposal is accepted, we'll work with release engineering to determine | ||
| a date to switch to cherry-picking. After a date has been picked, an | ||
| announcement will be sent out to the pulp-dev list with the planned date. After | ||
| tihs date, we should begin to follow this proposal. Also, we'll enter in Redmine |
pup-0003.md
Outdated
|
|
||
| This proposal would keep the current branching scheme[1] involving master, | ||
| x.y-dev branches, and x.y-release branches. The workflow by which changes are | ||
| moved between the branches would however change. |
There was a problem hiding this comment.
I think a clarifying sentence should be added at the end of this paragraph saying something like: The cherry picking process will apply to Pulp2, Pulp3, and future Pulp versions.
pup-0003.md
Outdated
| information externally to git. This means also having to remember to update | ||
| Redmine. | ||
|
|
||
| Switching to this model also creates some inconsistency. For example, if we |
There was a problem hiding this comment.
I think this paragraph can be eliminated because I'm hoping this will be added: https://github.com/pulp/pups/pull/3/files#r116336542
There was a problem hiding this comment.
I added the sentence in your comment but updated this to reflect the inconsistency within git history:
Switching to this model also creates some inconsistency. Looking at git history, you will see merge commits across branches (e.g. merges from the dev branch to master) up until the date in which we switch to cherry-picking.
Let me know if you disagree.
pup-0003.md
Outdated
| current devel branch, is likely to result in fewer bug fixes making it onto | ||
| devel branches. We can and should try to be diligent about cherry-picking fixes | ||
| to the devel branch, but the path of least resistance may prove too tempting | ||
| at times. That said, we could have a schedule (e.g. weekly) where one would |
There was a problem hiding this comment.
Since we're not doing this last sentence, how about removing it?
pup-0003.md
Outdated
| adopt this for Pulp 3, then we still have to use merge forwarding for Pulp 2. | ||
| Then we are stuck having to support two different strategies. | ||
|
|
||
| Also, when creating bug fixes in the cherry-pick model, code will be developed |
There was a problem hiding this comment.
I think this paragraph is just as true if we don't accept this PUP. That same awareness and scrutiny is required during the merge forward. In both cases it would occur cleanly and the 'extra scrutiny' is from the merger to know if any meaningful behavioral changes occured that don't show themselves as merge conflicts.
There was a problem hiding this comment.
I agree but I also feel like if we merge forward from dev branches, then we have more time to find bugs or bad merges on master since there's more time between master being released than a dev branch. Merging forward from a dev branch seems safer in this respect to me. I guess since we also added the part about NOT cherry-picking the change to the dev branch if there are conflicts that my point is somewhat negated. What do you think?
pup-0003.md
Outdated
|
|
||
| ## Footnotes | ||
|
|
||
| [1]: https://docs.pulpproject.org/en/2.12/dev-guide/contributing/branching.html |
There was a problem hiding this comment.
In the rendered doc the footnotes section is empty. I really like how these link, but maybe we could add another line so this shows up also in the footnotes section?
|
Updated. Thanks for the feedback. @bmbouter two items that have been collapsed but still need your input: |
|
(I left this comment elsewhere but it got crushed under the update) I think we should reconsider the "devel branch" terminology if we will be doing all development on master and cherry-picking backwards. Perhaps they would be better described as maintenance branches. |
|
Yea, this PR is getting unwieldy. I'm tempted to open a new one. I don't have a strong preference between calling them dev or devel branches versus maintenance branches although the connection between devel/dev and the actual branch name (e.g. 2.13-dev) is easier to pick up on. Looking at the other pulp documentation, it seems to just call them x.y-dev branches. Maybe we should use that? |
|
+1 to calling them x.y-dev branches.
I review the PR using the rendered version [0] and then if I have a comment
I find a line in that PR. When I do it that way, the comments don't get in
the way.
[0]: https://github.com/daviddavis/pups/blob/pup3/pup-0003.md
…On Tue, May 16, 2017 at 8:33 AM, David Davis ***@***.***> wrote:
Yea, this PR is getting unwieldy. I'm tempted to open a new one.
I don't have a strong preference between calling them dev or devel
branches versus maintenance branches although the connection between
devel/dev and the actual branch name (e.g. 2.13-dev) is easier to pick up
on. Looking at the other pulp documentation, it seems to just call them
x.y-dev branches. Maybe we should use that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3 (comment)>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AALAX1fTK4DmHHpe2j5-SEpKzsHJQuEHks5r6ZemgaJpZM4Mwhjp>
.
--
Brian Bouterse
|
pup-0003.md
Outdated
| 1. All bug fix PRs are opened against master | ||
| 2. After the PR is ready to be merged, the person merging the commit would | ||
| first merge the commit to master. Then, this person cherry-picks this change | ||
| from master to the latest x.y-dev branch. |
There was a problem hiding this comment.
Is the cherry pick pushed or is there a new PR open against the x.y-dev branch?
There was a problem hiding this comment.
If we make it 100% via PR (even if the PR needs no review) then we can disable pushing on branches entirely.
There was a problem hiding this comment.
Sounds good to me. Will update the PR.
pup-0003.md
Outdated
| person merging (who usually has better knowledge of which branches a commit | ||
| needs to be applied to). | ||
|
|
||
| We've also seen some serious mistakes where branches are accidentally merged |
There was a problem hiding this comment.
In the scheme of things maybe s/serious // ?
|
There's been no updates on this PUP in months. Status? |
|
After updating the "obvious consensus" terminology in PUP-1, we're going to rediscuss and revote on this. |
No description provided.