Esmvaltool: Provide guidance on using private repositories

[bouweandela]

Here is a proposed workflow for developing a diagnostic in a private repository:

1) Clone the private repository
For example, to clone a repository called esmvaltool-private, you would run:

git clone [email protected]:esmvalgroup/esmvaltool-private

or

git clone https://github.com/esmvalgroup/esmvaltool-private

2) Make a branch to develop your recipe and diagnostic:

git checkout master
git pull
git checkout -b my-awesome-diagnostic

3) Develop your diagnostic in that branch and push it to the private repository

git push -u origin my-awesome-diagnostic

the first time and

git push

any other time

4) Write and submit your paper

5) Push your branch to the public repository
Add the public repository as a remote

git remote add public [email protected]:esmvalgroup/esmvaltool

or

git remote add public https://github.com/esmvalgroup/esmvaltool

and push your branch to the public repository

git push -u public my-awesome-diagnostic

6) Make a pull request in the public repository
go to https://github.com/esmalgroup/esmvaltool/pulls and click the 'New pull request button'. Process reviewer comments and get it merged.

7) Obtain a DOI for your code and add it to your paper
Wait for (or request by opening an issue) a new release of ESMValTool. With the next release, your diagnostic recipe and source code will automatically be included in the archive on Zenodo and you can add the DOI from Zenodo to your paper: https://zenodo.org/record/3698045

[bouweandela]

@ESMValGroup/esmvaltool-coreteam Could you please review this and provide comments? Once we agree on a workflow, this can then be added to the documentation.

[valeriupredoi]

Two comments:

• add point to enter github username+password after git clone (required step only for private repos)
• remember to pull the origin everytime you start working again in case someone else made changes in the meantime
  Cool, B-man! :beer:

[ledm]

Do we need to include a test/demonstrate that the recipe works as a requirement? I guess it falls in point 6, but should probably be made explicitly clear here.

Is this the process that we'll be using for merging AR6 code as well?

[mattiarighi]

Looks good to me, thanks!

[bouweandela]

Do we need to include a test/demonstrate that the recipe works as a requirement? I guess it falls in point 6, but should probably be made explicitly clear here.

I think this will be done as part of the pull request review and you can then update the figures in the paper before it gets published if the review results in major changes. Maybe we could add a link here to the contribution guidelines?

[nielsdrost]

@bouweandela Yes, this is pretty much the original plan. At the time we also had the idea/option to merge to master/develop in the private repo, but in hindsight this was a bad idea. ;-)

[BenMGeo]

I like it. I actually did not realize that private branches should directly be pulled into public.
Though, I'd mention to frequently update from master.

[zklaus]

Overall that could work, I think.

That means private/master is identical to public/master?

* add point to enter github username+password after git clone (required step only for private repos)

I don't think we need to write that. If necessary, the tools will ask.

I like it. I actually did not realize that private branches should directly be pulled into public.

Clarification: According to the guide above, private branches should become public branches before they get their (also public) pull request.

Several people suggested frequent updates from master. I generally follow a different philosophy, that is one of the major approaches out there, which is to finish your development with the master version that you started with and then integrate everything before the PR. Otherwise it's too much of a moving target for me.

[axel-lauer]

I followed this workflow for getting the branch 'russel18_v2' from the private into the public repository and create a pull request (#1589). That all worked fine but the commit history now contains more than 10,000 commits resulting from the automated synchronization between the master branches in public and private, e.g.:

@bjoernbroetz Merge branch 'master' into private-branch 451a74a
@bjoernbroetz Merge branch 'private-branch' into autosync 775b0ea

This seems rather undesirable to me but I do not know what to do about this. Does anyone think this is a problem and if so does anyone have an idea what to do?

[zklaus]

If we really follow this workflow it seems to mean that

• merges occur only in the public master, never in the private one
• master in private and public are identical
• all (feature) branches in private branch off of the (public) master

If that is the case, there is no need to maintain a separate master branch in the private repository. Instead, we could simply push the public master to the private repository (handled as a separate remote on one machine).

[bouweandela]

master in private and public are identical

I checked and this is the case

If that is the case, there is no need to maintain a separate master branch in the private repository. Instead, we could simply push the public master to the private repository (handled as a separate remote on one machine).

Indeed, this is a good idea. @bjoernbroetz Would it be possible to update your synchronization script so it pushes the public master branch to private repositories instead of works with pull requests that create noise in the git log?

[bouweandela]

I followed this workflow for getting the branch 'russel18_v2' from the private into the public repository and create a pull request (#1589). That all worked fine

Great to hear that it worked!

but the commit history now contains more than 10,000 commits resulting from the automated synchronization between the master branches in public and private, e.g.:

@bjoernbroetz Merge branch 'master' into private-branch 451a74a
@bjoernbroetz Merge branch 'private-branch' into autosync 775b0ea

This seems rather undesirable to me but I do not know what to do about this. Does anyone think this is a problem and if so does anyone have an idea what to do?

I just did these steps and made a new pull request here #1592:

git checkout master
git pull
git checkout russell18_v2_public
git diff master... --binary > patch.bin
git checkout master
git checkout -b russell18
git apply patch.bin
git add doc esmvaltool
git commit --author "amarjiitpandde <[email protected]>"

that way you loose the git history, but at least you get rid of the 10000 needless commits. I hope it's not a problem, if it is, feel free to close it again. Once @bjoernbroetz has updated his synchronization scripts, we should be able to use the normal procedure.

[zklaus]

We do need to rebase all open branches in the private repo then, right?

Or maybe rather subject them to the same treatment that you lay out for the russel branch.

[bouweandela]

We do need to rebase all open branches in the private repo then, right?

Or maybe rather subject them to the same treatment that you lay out for the russel branch.

I guess so, unless you know something smarter with git? I tried rebasing first, but it didn't work straightaway, therefore I went for the simpler approach instead.

[bouweandela]

We could also consider not having a master branch in the private repositories, but maybe that would increase the risk of people accidentally pushing stuff to the public repository. Anyone an opinion on that?

[bettina-gier]

I'd personally suggest to just push the same master to the private repository if that isn't a big problem. I think that's easier for a simple user who may still be struggling with git, and who are quite wary of anything that has to do with the remote repositories cause they "don't want to screw anything up".
Because otherwise it's more added instructions on how to get the public master, setting up a second remote in your local git repository etc. We've already heard enough complaints that the entry bar into ESMValTool and setting it up is quite steep, so let's try not adding anything onto that if we can avoid it.

[zklaus]

Or maybe rather subject them to the same treatment that you lay out for the russel branch.
I guess so, unless you know something smarter with git? I tried rebasing first, but it didn't work straightaway, therefore I went for the simpler approach instead.

I agree with this approach. Note that this will require a forced push as well, which might affect people working on those branches. I counted about 80 open branches right now.

[zklaus]

I made a list of the open branches together with the person (name and email) who made the last commit in that branch.
xxx

[bouweandela]

Note that this will require a forced push as well

It might be safer to advise people to simply make a new branch off the master branch and apply the patch to that, as in the example. That way there is no risk of losing work.

[zklaus]

At least a reduced risk :) Alright. I updated the list to include the commit date of the branches. We might want to set a time limit until when this has to be done.
xxx

[bjoernbroetz]

So, we will make a forced push of the master branch of the public to the master branch of the private repository. This will eliminate the 10.000 useless merge commits that are currently in the master of the private. Then I will use a basic git push private master to bring automatically the public master to the private. @bettina-gier is right about the issue of keeping things simple for the diagnostic-developers. That was the idea back in time when we thought about remotes vs a local copy of the master branch.
Now that we have squash merge (#1653) in the public we don't need to worry more about the useless merge commits that linger in the active private branches. The only negative effect they will have is that I will appear as co-author of all those squash merged PRs ... maybe we can write a filter for that.

[bouweandela]

The only negative effect they will have is that I will appear as co-author of all those squash merged PRs ... maybe we can write a filter for that.

Yes, you can edit that out when writing the commit message for the merge. I guess the box for the message will get pre-filled with a very long text though, but let's deal with that when it happens.

[axel-lauer]

OK, so I just made this forced push. Fingers crossed...

[bouweandela]

Looks like it worked :tada:

[bouweandela]

@bettina-gier This could be interesting for you.

[bouweandela]

@bettina-gier @schlunma @rswamina @hb326 This is the issue I mentioned with a possible procedure for getting things from the private repos used for developing papers to being included into the ESMValTool. Could you have a look if this would work, probably with some adjustments? Maybe we can make a pull request and add this to the guidance here: https://docs.esmvaltool.org/en/latest/community/?

[hb326]

@bettina-gier @schlunma @rswamina @hb326 This is the issue I mentioned with a possible procedure for getting things from the private repos used for developing papers to being included into the ESMValTool. Could you have a look if this would work, probably with some adjustments? Maybe we can make a pull request and add this to the guidance here: https://docs.esmvaltool.org/en/latest/community/?

@bouweandela: I just tested if I could push a branch from the private to the public repository. It worked fine for me with the description you had provided. I think we can open a pull request here.