Yannick opened the meeting by reminding all participants that we are in the midst of preparations for our upcoming public release so there have been many recent pull requests and merges. Some of these have the nature of clean-up and require minor changes in model repositories. An example is Oops PR #1076 which replaces boost::shared_ptr with std:shared_ptr and thus requires changes in the geometry interface.
Yannick also announced that JEDI contributors will soon have a day off from JEDI contributions: As explained below, Friday, October 16 is Migration Day. This is when we will be doing some restructuring of the JCSDA GitHub repositories in preparation for the release. We will disable any new pushes, pull requests, or ZenHub issue creation on the JCSDA repositories that day.
Mark then presented the following slides which outline the time frame and logistics for the release of JEDI and CRTM.
The most important dates are October 16 (Migration Day), October 23 (code freeze), and October 28 (selected jcdsa repos become public).
Mark described some of the logistics (see slides for details). In short, the plan is to create two GitHub organizations. On migration day, we will rename the current jcsda organization to jcsda-internal. And, we will create a new organization called jcsda that will host the public repositories.
Also on Migration day, we will convert selected jcsda-internal repositories to be forks of the repositories on jcsda. So, for example, github.com/jcsda-internal/oops will be a fork of github.com/jcsda/oops. For a complete list of the repositories that will become public (with private forks on jcsda-internal), see slide 4.
The reason for doing this is because GitHub will not allow users to create a public fork of a private repository. So, this means that the public repository must be the base and the private (jcsda-internal) repository must be the fork.
Please take care to not push anything to github on Migration Day (slide 3) and take note of accessing the newly named organizations after the 16th (slide 5).
Though converting the jcsda-internal repos to forks of their jcsda counterparts is straightforward from a GitHub perspective, the operation is made a bit more complicated by our use of ZenHub. We wish to keep all of our ZenHub issues on the private repos. So, converting these repos into forks without losing the ZenHub issues will be a technical challenge. We are working with ZenHub to sort this out before Migration Day.
As part of this migration procedure, we will also delete stale branches. We define a stale branch as any branch that has seen no activity for three months, since July 15, 2020. So, it is very important that you Claim your active branches - let us know if there are any feature branches that you have not pushed to in the last three months but that you still want us to keep.
Rick then asked how this was going to be announced and Mark responded that all should consider this meeting as the announcement. Mark also posted an announcement on the GitHub JEDI team page and he created a google document as a way for you to claim your active branches. You can edit the document directly or you can email one of the JEDI core team and we will edit the document for you.
But, you do not have to wait for us to delete unused branches. We encourage everyone to delete any of their own branches that they no longer need. Every GitHub repo has a drop-down menu where you can select a particular branch to view. Just to the right of that menu there is a link to a list of branches for that repo. When you select that link, you see the list with your own branches highlighted. Please delete any that you no longer need.
This must be done by October 15 - the day before Migration Day.
If any branches are mistakenly deleted, there is no need to fret. If you still have a copy of the branch on your local machine, you can recreate it by pushing it to jcsda-internal.
Mark O asked if branches need to be claimed for all JCSDA repos. The answer is no. We will only be pruning the repositories that will migrate to the new jcsda organization - those listed on slide 4.
Ben asked about how lfs files will be managed as part of this procedure. Mark M responded that this is a good opportunity to remove unneeded large files from the git history. But, beyond that, the handling of lfs files should be transparent. There may be a transition time when we have multiple copies of lfs files on the remote lfs data store. But, after the jcsda-internal repos are converted to forks of the jcsda repos, there should not be duplicated lfs files. Both the parent repo and the fork will point to the same objects where they overlap.
Ben also asked if you have a currently open PR that will end up being a PR into the new public repo, how should that be handled? The procedure here will be to temporarily close the PR before Migration Day, then re-open the PR after the jcsda-internal repo becomes a fork of the public repo. The fact that jcsda-internal will be a fork of the public repo will issue the PR into the public repo develop branch when it is re-opened. It will be acceptable for the public develop branches to evolve after Migration Day.
JJ asked about how this will affect repos that are not part of the release, such as mpas-bundle. Mark explained that they will remain on jcsda-internal and will remain private until they are part of a future release. This may require some minor changes. For example, the CMakelists.txt files in the bundles will now have to specify jcsda-internal as the organization for each repo.
Mark mentioned in his presentation that only those included in the AOP will need access to jcsda-internal and others can work with the public repos. There were several questions about this. Mark and Yannick assured everyone that nobody will lose access immediately. The main motivation behind this effort is to remove access for individuals who are not actively contributing to the code. We currently have many individuals in our GitHub organization that have rarely or never pushed to any of the repositories. We wish to limit access to active contributors. Access to jcsda-internal will be considered on a case-by-case basis and exceptions are possible as long as people are actively contributing.
Yannick noted that since we are creating public repositories for the first time for this initial release of JEDI, subsequent releases (of which will have existing public repos) will be much simpler.
This concluded the discussion about the release so we then moved to a presentation by Rick on best practices for code reviews.
Rick began by emphasizing that these are his own perspectives and we are open to other opinions on what makes for a good code review. He also emphasized that the value of code reviews should not be underestimated - it is substantial work that all team members are encouraged and expected to engage in. He also emphasized owning responsibility for your reviews.
He spoke about using templates to guide the review process. In response to a question by Mark he elaborated that this may include a template for the author of the pull request (PR) as well as a template or guidelines for the reviewer. Important items to include in the PR description include a precise definition of what it means for this issue to be considered "Done". And, a good PR description can help guide the reviewer. For example, explaining why a certain feature was implemented may help the reviewer to suggest an alternative way to achieve the same goal.
Ben pointed out that a common situation is when developers contribute code but do not add one or more tests that will test that code. Rick answered that this is why we have the code coverage tests in many of the JCSDA repos. Reviewers should pay attention to these automated tests and if they fail, they should encourage the author to write a test for the contributed code.
Chris H then asked about the size of PRs. In keeping with established best practices in agile software development, the JEDI team has always encouraged small, focused PRs that address a specific issue. These are more likely to be reviewed and merged faster than large PRs that are difficult to review. But, Chris asked whether it makes sense sometimes to bundle a few small, unrelated changes into one PR. Participants generally agreed that this is efficient and often a good idea, provided that it meets a few criterion. Clearly the first criterion is that it is small and relatively easy to review. A second criterion suggested by Travis is that the PR has an appropriate title. For example, it is not good if a PR has a title like "this fixes issue X", but in addition it has a number of other small changes that are unrelated to X but that are "piggybacking" on that PR. For the types of PRs that Chris is referring to, a better title might be something like "Clean up...". Tom also added that the PR author should consider the expertise required to meaningfully review the PR. For example, if the unrelated changes require expertise from different people to understand what is being done then the author should consider splitting them up into different PRs. Anna also added that splitting up PRs can also help with velocity charts and planning. For example, if some changes are relevant to DA (JEDI 4) and others to infrastructure (JEDI 1) or documentation (JEDI 5), it might be good to split them up into different PRs.
Mark encouraged Rick to create a ZenHub issue for including a document in the "Best Practices for Developers" section of the documentation that provides guidelines for giving a good code review. It may be organized into questions the reviewer can as herself/himself during the code review such as "Can this functionality be achieved in a simpler way?", "Have I made it clear why I am requesting these changes", etc.
Rick closed by providing links for further tips and discussion from the software community on best practices for code reviews (see also slide 6):
https://www.michaelagreiler.com/code-review-blog-post-series/
https://google.github.io/eng-practices/
https://www.atlassian.com/agile/software-development/code-reviews
Chris H agreed these could be useful but cautioned that a software development environment like Google DevOps may be quite different than our software environment so we should be selective on what we take away from this.
Yannick encouraged all meeting participants to take a look at some of these articles (Homework!) and advocated for another meeting in the future on this topic where we can take some of these insights, apply it to our development process, and document it to help new team members.
Anna, Yannick and others closed the meeting with an encouragement for all to actively engage in code reviews. Even if you are not intimately familiar with the sections of code they address, reviewing a PR can help you learn a great deal about the code. It is a great way for newcomers to become familiar with the code and it is a great way for experienced users to broaden their knowledge across more of the code base. A question also came up on whether it is ok to review a PR even if you are not asked to. All JEDI answered with an emphatic Yes! There can never be too many reviewers - the more eyes on a code the better. So, no need to wait for someone to invite you - go ahead and review any PR that looks interesting or that addresses a segment of code that you would like to understand better.