Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instructions for creating backport PRs #36593

Merged
merged 7 commits into from
Feb 23, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Feb 22, 2018

SUMMARY

Add instructions for creating backport PRs

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

docs/docsite/rst/community/development_process.rst

ANSIBLE VERSION
devel
ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Feb 22, 2018

@ansibot ansibot added docs_pull_request needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 22, 2018
@sivel
Copy link
Member Author

sivel commented Feb 22, 2018

This needs reviews from the RMs too. Added as requested reviewers.

@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label Feb 22, 2018

git checkout devel
git --fetch upstream
git merge upstream/devel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually a pull --rebase is better to make sure any local changes are 'on top', but this is actually not needed at all since we are updating stable-2.5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, part of this is based around my workflow, and my devel is never diverged from upstream/devel, so a git merge upstream/devel always applies correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he, i always diverge devel, then push to diff branch

git --fetch upstream
git merge upstream/devel
git checkout stable-2.5
git pull
Copy link
Member

@bcoca bcoca Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be --rebase to ensure local changes are applied to the top
this also assumes stable-2.5 was already preexisting on the local repo, i would instead use this step and leave the other as an 'option' if you already have stable-2.5

git checkout upstream/stable-2.5 -b stable-2.5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't necessarily assume stable-2.5 was already pre-exisiting. The first time you try to checkout a branch, if it's not local, it's pulled from one of the remotes.

In which case, you should never need --rebase, since that branch is from upstream.

But yes, there are a lot of assumptions when trying to talk git workflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had issues with the branch autoselection, but probably cause i have multiple remotes with the branch, in any case you should specify which one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if checking out new, there is no need for pull, if the branch existed locally, doing pull w/o a --rebase can create issues (user had local commits)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end you can reduce most of these lines to 2 steps

       git --fetch upstream
       git checkout upstream/stable-2.5 -b cherry-pick/2.5/[PR_NUMBER_FROM_DEVEL]

git checkout stable-2.5
git pull
git checkout -b cherry-pick/2.5/[PR_NUMBER_FROM_DEVEL]

Copy link
Member

@bcoca bcoca Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this step is needed, since the instructions assume devel has the commit as seen below

or is this intended to work as a rename? -m ? could just checkout upstream/stable-2.5 directly into the desired name

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand this comment. The idea is that we are creating a feature branch from a stable branch, cherry picking into the stable feature branch from devel, so we can push and create a PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, checkout is not really best way, it seems you just want -m (rename)


.. note::

The choice to use ``cherry-pick/2.5/[PR_NUMBER_FROM_DEVEL]`` as the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bikeshed: i would recommend 'backport' instead of cherry-pick as sometimes the PR will not be possible to be made by cherry-picking (too much code changed).


.. note::

These instructions assume that ``https://github.com/ansible/ansible.git``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and that the origin remote refers to your personal Ansible fork.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he, posted 'edit' with that

@bcoca
Copy link
Member

bcoca commented Feb 22, 2018

posted a couple of edits as suggestions, feel free to blow away

@ansibot
Copy link
Contributor

ansibot commented Feb 22, 2018

The test ansible-test sanity --test rstcheck [explain] failed with 1 error:

docs/docsite/rst/community/development_process.rst:48:0: Unexpected indentation.

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 22, 2018
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 22, 2018
#. Prepare your devel, stable, and feature branches:

::
git --fetch upstream
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a blank line here to fix formatting

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

@dharmabumstead dharmabumstead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks @sivel! Merge at will.

.. note::

These instructions assume that ``https://github.com/<yourgithubaccount>/ansible.git``
is configured as a `git remote`` named ``origin``. If you do not use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing ` in-front of git remote

@gundalow
Copy link
Contributor

Tested and worked for me.
Has anyone automated this yet (given a PR, find commit, create PR)

@sivel
Copy link
Member Author

sivel commented Feb 23, 2018

Has anyone automated this yet (given a PR, find commit, create PR)

I have not. It wouldn't be too hard, but the changelog part would be omitted, unless whatever did it paused waiting for you to confirm, or tried to do it on it's own with assumptions.

It would also have to be tolerant of merge conflicts.

Going to proceed with merging this as is.

@sivel sivel merged commit 076ea07 into ansible:devel Feb 23, 2018
gundalow pushed a commit to gundalow/ansible that referenced this pull request Mar 1, 2018
* Add instructions for creating backport PRs

* Update development_process.rst

simpler workflow

* Update development_process.rst

added origin note

* A few adjustments to clarity, use backport instead of cherry pick in branch name

* Address formatting issue

* fetch isn't a flag

* Fix rst formatting

(cherry picked from commit 076ea07)
@ansibot ansibot added docs This issue/PR relates to or includes documentation. and removed docs_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
docs This issue/PR relates to or includes documentation. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants