FHIR Chat · Simple steps for pull requests and merging

Stream: committers

Topic: Simple steps for pull requests and merging

Vassil Peytchev (Feb 25 2021 at 18:50):

I hope the following may help people better navigate git and GitHub (within the context for master and R4B)

Background
As far as git is concerned, master and R4B are just branches in the FHIR repository. Github layers functionality on top of git for features like pull requests to help with managing repositories.

Pull requests in Github can be done from a branch, or from a fork. The description below is for pull requests from a branch. As far as I know, forks are not used in the current process.

First time
Clone the repository - note that it will bring information about all branches. I prefer using ssh, so the command is

git clone git@github.com:HL7/fhir.git

Using a URL, it would look like this:

git clone https://github.com/HL7/fhir.git

Which branch?
When you first clone the repository, you are placed in the master branch. If you need to do work that will go in the R4B branch, you can switch to it using

git checkout R4B

Now, make sure you can successfully build the branch you are interested in, to confirm that your local setup is working.

Creating your local development branch
You can confirm that you are starting from the correct branch using the git branch command, which will list all local branches, and show you which the currently checked out branch is. At this point you can create you own branch:

git checkout -b myCoolBranch

You will be in the new branch, and ready to make your changes

Committing your changes
New files need to be added explicitly with the git add command. Once added, subsequent changes can be committed with a single command:

git commit -am "helpful message"

Re-sync with original branch
The following commands will make sure any changes to R4B (or master) since the last time you pulled them from Github are accounted for

git checkout R4B
git pull origin R4B
git checkout myCoolBranch

If there were changes to R4B (or master), you must merge them to your branch (make sure that you are in your development branch, in this example myCoolBranch):

git merge --no-ff R4B

The --no-ffflag is optional, it preserves individual commits - I don't know if this is helpful or not in the long run.

Build your branch
This will maximize the chances that the changes are going to be successfully merged in the end.

Push your branch

git push origin myCoolBranch

Create a pull request
Now, if you got to Github you will see your branch and you can create the pull request.

I believe following these basic steps will make the process less prone to encountering issues. If something is wrong, or can be improved, please chime in.

Jean Duteau (Feb 25 2021 at 19:11):

This is wonderful advice to people who may not be experienced with git and branching. I do note that we need an equivalent article for people who use forks as I see that many people have been doing that.

Vassil Peytchev (Feb 25 2021 at 19:22):

Given that forks are for taking the content and doing something different with it, rather than managing the development of the content, maybe we should be more explicitly discouraging the use of forks?

I definitely don't know enough about it to make suggestions on making pull requests work from forks.

Jean Duteau (Feb 25 2021 at 20:00):

the issue with discouraging forks is a) people have been told to do that rather than branch and b) branching requires committer access so we'd need to be given committer access to more people. Neither of those are showstoppers.

Mark Iantorno (Feb 25 2021 at 20:03):

I have enabled forking access to the secure files in the pipeline

Mark Iantorno (Feb 25 2021 at 20:03):

This should no longer be an issue

Vassil Peytchev (Feb 25 2021 at 21:51):

@Lloyd McKenzie @David Hay - is using fork-based pull requests something that FMG needs to address? I have no idea if there are any implications whatsoever, and it could be that is has already been discussed and dealt with.

Yunwei Wang (Feb 25 2021 at 22:19):

If needed, I could provide my git commands for fork users

John Moehrke (Feb 25 2021 at 22:22):

this is what we have today https://github.com/hl7/fhir/wiki/Get-Started-with-FHIR-on-GitHub

Lloyd McKenzie (Feb 25 2021 at 23:28):

I'll admit to not really understanding the ramification of branch vs. fork. @Josh Mandel Do you have advice here?

Mark Iantorno (Feb 25 2021 at 23:29):

Everyone can use forks, it is no longer an issue

Mark Iantorno (Feb 25 2021 at 23:30):

The issue was that I need to use secure keys to upload the built version to build.fhir.org. Azure Pipelines, by default, restricts access to the private keys. Only pull requests originating from the actual repo, not forks, can access those secret keys. They recommend this as best practice.

Mark Iantorno (Feb 25 2021 at 23:31):

I did not know this.

Mark Iantorno (Feb 25 2021 at 23:31):

I have since gone in and enable key access to all PRs, including forks.

Josh Mandel (Feb 25 2021 at 23:31):

Wait, so you are saying you have chosen not to adhere to this best practice?

Mark Iantorno (Feb 25 2021 at 23:33):

The choice is either to expand the list of contributors to the repo, (or open it to all contributors), or allow fork PRs to access the keys. After reading through the documentation, I don't think there is any reason to not allow the fork PRs to run on the pipeline.

Mark Iantorno (Feb 25 2021 at 23:34):

I monitor the pipeline, and get notifications of all activity, so I will see if anything is up.

Josh Mandel (Feb 25 2021 at 23:34):

I mean the risk is that somebody exfiltrates keys and uses them at will.

Josh Mandel (Feb 25 2021 at 23:35):

This keys can be used to replace all content in build.fhir.org at least -- it's worth thinking through what else could go wrong.

Mark Iantorno (Feb 25 2021 at 23:36):

Yeah, I understand. If you feel that it's not the right approach, I can disable it.

Mark Iantorno (Feb 25 2021 at 23:36):

The alternative though is maintenance of the list of contributors, which is fine.

Josh Mandel (Feb 25 2021 at 23:36):

We already have a model where only committers are allowed to contribute to the source tree, for intellectual property reasons as well as maintenance reasons. I'm just trying to figure out what we are accomplishing by opening up to editors outside.

Mark Iantorno (Feb 25 2021 at 23:37):

This seemed to be the way people were doing PRs so I assumed it was standard practice>

Mark Iantorno (Feb 25 2021 at 23:37):

How were people contributing before?

Josh Mandel (Feb 25 2021 at 23:38):

You mean before we moved to GitHub?

Josh Mandel (Feb 25 2021 at 23:38):

They needed to be registered as subversion users in a system called GForge

Mark Iantorno (Feb 25 2021 at 23:38):

Okay, so I'm fine with saying not to do forks

Mark Iantorno (Feb 25 2021 at 23:39):

Is that okay with you @Josh Mandel

Mark Iantorno (Feb 25 2021 at 23:42):

I just disabled it

Rob Hausam (Feb 25 2021 at 23:43):

It seems like that's probably the best and easiest solution for this.

Mark Iantorno (Feb 25 2021 at 23:44):

Everyone just use regular branches, and if they want to be added as contributors let me know and I can do it

Rob Hausam (Feb 25 2021 at 23:45):

I use forks and create PRs in GH all the time - but not with a build pipeline like this.

Yunwei Wang (Feb 26 2021 at 02:03):

The instruction is confusing and conflict. Is this final? @Josh Mandel @Lloyd McKenzie
Please add my user name yunwwang to the contributor. @Mark Iantorno BTW, do I need to re-create my PR?

Mark Iantorno (Feb 26 2021 at 02:04):

You will need to clone the main repository and create a pull request off the R4B branch

Yunwei Wang (Feb 26 2021 at 02:05):

The one I create this afternoon already passed system test. Can I merge that after I have my contributorship?

Mark Iantorno (Feb 26 2021 at 02:06):

If the option to merge is there then yes, if not, then it would be best to re-create the PR

Mark Iantorno (Feb 26 2021 at 02:06):

I send you an invite

Yunwei Wang (Feb 26 2021 at 02:08):

I can merge my PR now. Thanks

Lloyd McKenzie (Feb 26 2021 at 05:31):

@Mark Iantorno If you're going to add new committers, make sure you have an email from them indicating that they agree to abide by the IP chapter of the HL7 GOM and also to actively monitor #committers/announce