Improve hotfix documentation #322
Conversation
|
|
||
| ### Bug appears on staging during release cycle | ||
| - Branch of off the staging branch |
|
|
||
| ### Bug appears on stable outside release cycle | ||
| - Branch of off the stable branch |
|
|
||
| ### Bug appears on stable during release cycle | ||
| - Branch of off the stable branch |
| When a bug needs to be hotfixed on stable outside of the release phase the hotfix needs to be based on stable and then a PR needs to be made for stable and master | ||
|
|
There was a problem hiding this comment.
Can you please add some commas to all these sentences where appropriate, to make them easier to parse
There was a problem hiding this comment.
A single comma could be added after the qualifying phrase like so:
When a bug needs to be hotfixed on stable outside of the release phase, the hotfix needs to be based on stable and then a PR needs to be made for stable and master
However, I believe that the underling issue that is the sentence is too verbose. From this point on @juliangiebel, please take all this with a grain of salt as it is purely my opinion.
- Future tense is necessary here "needs to be" and should be replaced with "is."
When a bug is hotfixed on stable outside of the release phase, the hotfix is based on stable and then a PR is made for stable and master
- Provide instruction not observation. Just tell the reader what to do.
To hotfix a bug on stable outside of the release phase, base the hotfix branch on stable, and make a PR to both stable and master.
- Do not include the definition as appositives (things that have already been implied). If you are hotfixing it is implied that a bug is being resolved and that this is outside of release phase.
To hotfix stable, base the hotfix branch on stable, and make a PR to both stable and master.
There was a problem hiding this comment.
Line 19 and potentially 17 are also rather runaway
| ```shell | ||
| git checkout staging | ||
| git checkout -b "<hotfix branch name>" |
There was a problem hiding this comment.
Would it be "needless spoonfeeding" to also mention that one must also first pull their branch from upstream? It sounds obvious but I actually did make that mistake, working under pressure in an unfamiliar scenario is weh
Jezithyr
added
Minor Changes Needed
The whole reason for this procedure is to not have to cherry pick commits. |
|
As mentioned in the maintainer meeting... It looks like cherry-picks are required for both hotfix schemes PR into Stable and cherry pick from Master. Red is the hot fix in the diagram. Both schemes have problems. Git does not store diffs so merge conflicts should not be created by cherry picking from master into stable. For the merge strategy, changes parallel to the hotfix can occur causing conflict when the return pr from stable to master is made requiring cherry-pick or elaborate branching and merging: branch off stable, merge master into stable, resolve conflict, create PR into master. If a hotfix using cherry pick into stable from master has a conflict, it is a strong indicator that the cherry pick was done incorrectly. All cherry-picks should not depend on other changes in master which are not present on stable. Conflicts can also indicate that hotfixes with file overlap have been merged in a different order than originaly. If it is the case that merges from master to stable should always fast forward, then PRs into stable break this guarantee by allowing parallel changes. Resolution of such conflicts will require cherry picking or previously mentioned elaborate branching and merging. If it is not the case that merges from master to stable should always fast forward, then patching main then patching stable with a cherry pick will be inhibited. This is based of the assumption that patch environment should be the sable branch to avoid unnecessary pollution. Developers pull current stable (on fork) make a PR into master this commit is then cherry picked into stable. Since stable is not a guaranteed fast forward merge, conflicts with the cherry pick may occur. I believe that these are effectively mutually exclusive strategies |
The wording wasn't consistent with the rest of the doc and the explanation wasn't clear enough