CDM - Development Operating Model - PR Labels and processing

[iansloyan]

Here I propose a process for labelling of Pull Requests by contributors, assessing PRs by maintainers: reviewing, approving and discussing as necessary.

This needs to be agreed by CDM maintainers. Examples are given and more PRs can also be run through this matrix to find a workable matrix of labels and actions to make this efficient and comprehensive.

Timings or a service level agreement from the maintainers should also be proposed and discussed as part of this development operating model.

FINOS Maintainers Issue and Pull Request Approval Process.pptx
Pull Request processing examples.xlsx
(The excel file is just to store examples of how a PR would be tagged when the labels are implemented - not intending to use offline spreadsheet!)

[iansloyan]

@finos/cdm-maintainers

Please review this draft proposal - edits and alternative ideas welcome - particularly those which simplify. Feel free to make edits to the files and upload your own ideas or comment below.

[lolabeis]

👍

[lolabeis]

Requiring that an issue be raised prior to making a pull request in most cases (maybe except for bug fixes, security patches or infrastructure upgrades) would be extremely useful in ensuring that there is a clear business-friendly description of the purpose of a PR. It would help triage massively.

The structure of an issue could be pre-populated with a template to guide users on the raising process. The "issue card template" that has been used for several years for CDM development should provide a good base.

[chrisisla]

Some thoughts:

I like the labels but I personally would struggle to decide what label to put on any particular issue/PR. There is also the issue that one maintainer may think something is critical, but another may think it is optional. This could be based upon a different level of experience, different level of knowledge in the area be changed for example. So if we were to start adding labels then I think it would need to be done as a group to ensure the correct labelling is applied.

I think there are too many options as to which and how many maintainers should review an item. It's too complicated. Can we not just say 2 maintainers for every PR as a minimum, and they have to have a knowledge of the area being reviewed? So for example, if I review a change for derivatives all I am doing is looking to make sure it does not break anything that is used by securities lending; I do not have enough expertise in derivatives to review the actual change itself. It would be better if every PR was reviewed by people with an understanding of all 3 industries but this is probably not feasible.

I like the idea of having templates for our Issues, I think we should create something similar to what Rob and Eteri showed us on a call the other day.

I like the idea of using milestones to help manage release content and schedules. Not really associated to what we're looking at here but I think if we use milestones we can possibly simplify the labelling (as we can just assign to a milestone rather than add certain labels perhaps).

In general I think we've been looking at how we can manage this as an internal agile software development project, which is really how it has been run up until now. But the CDM is not that sort of project now really, it's now an open source project, where we can get contributions from anyone at any time with or without Issues being created. I agree we need to somehow manage this but we need a much more streamlined process, as at the moment the processes described seem to put too large an onus on the maintainers I feel.

[tomhealey-icma]

My final comments are:

1. I like the idea of using an issue template but I'm concerned about the workflow. If you create an issue and then a PR, should maintainers comment the issue or PR? I would suggest giving it a try for large modelling changes and then if needed we can expand it for other types of PRs. The issue is only for reference and would not necessarily determine the outcome of the PR.

2. I disagree with the maintainer selecting persons to review. I would want to have any of the primary maintainers/reviewers to have the option to review. My point here is that even if you are not a domain expert we need to agree on a set of design principles that will guide contributors and reviewers. Related to this I would also suggest that each month one review would have the responsibility to review new PRs and validate the labels and other criteria before other reviewers would need to proceed. A label, "Passed Operational Rules" would be added.

3. I don't think documentation should be required with the PR. It can be added later as a separate PR.

4. While the labelling appears complex, if we can manage it, I think it will better help us review, approve and process PRs.

[dschwartznyc]

This is a fine start - ty Ian.

Comments:

Confirming the goals of the process as a baseline. Proposed are:

• Encourage the adoption of CDM
• Support timely commercial innovation
• Ensure stability and supportability of production-released code (adherence to standards, non-breaking changes, and the ability to roll back)

From a cursory review of open-source feature/release management, there seem to be at least two approaches:

• Features are released based on a community vote (add, neutral, don't add). A change with a pre-determined minimum number of votes is added.
• There is centralized governance of the codebase which explicitly accepts changes.

The second seems more appropriate given the objective that CDM becomes mission-critical to Capital Markets.

I suggest that the focus of the governance process should shift to the review of changes (see the attached for an overview). This shift should create scale by removing the potential that the WGs become a bottleneck to starting work and by removing the bottleneck that could come from the Contribution WG having to conduct detailed functional reviews.
It does place the burden on the WGs/maintainers to review changes.

Under this approach:

• anyone can suggest a change by raising an issue.
• anyone can work on a suggested change. Those planning to develop the change can use the WG to brainstorm/get support for the idea and/or request others to collaborate in the process. Once the item is completed, a PR is raised. The PR categorizes the change according to urgency, complexity, and the WG(s) that need to review.
• one or more maintainers have to review and approve the PR. The scope and complexity of the change determine the number and breadth of the review. Requests to overturn a denied approval can be escalated to the Steering WG.
• A PR that has been approved by the relevant WG(s) (modeling or technical) enters the queue for final approval in the Contribution WG. That approval is not based on a domain-focused feature review but rather driven by a checklist that confirms the approval artifacts are in place. In almost all cases, PRs that are not approved by a WG are not considered for release.

Modeling / Technical WG review/approval begins with triage to confirm the categorization of the PR (urgency, complexity, scope alignment). Approval is based on:

• the consistency of the change with CDM's commercial and architectural goals
• the completeness of the code
• adherence to coding standards: formatting, comments, comprehensiveness of test cases, et al
• the contribution passing unit and integration tests
• other relevant criteria

CDM Change Management.pdf

[tomhealey-icma]

At the prior CRWG meeting we agreed to finalize the development operating model #2129, by today, June 2nd. I’ve Summarized the comments that I believe we’ve agreed to and will move forward with:

1. We will work to create a template for contributors to use. Until the templates are ready, users will follow the existing structure and provide as much detail as possible to expedite review and approval of a PR.
2. Contributors will be required to open a Git Issue with detailed background information on the proposed change. This should include business cases and examples. I had proposed limiting this to large modelling changes only and not all PRs.
3. PR modelling changes must follow the design principles and guidelines.
4. All maintainers will have the opportunity to comment on and review PRs.
5. Documentation can be submitted in a separate PR after the model and code change PR.
6. The policies will go into effect on Monday June 12th. All existing PRs are grandfathered in, but PR owners will need to re-tag their projects.

Please provide final comments.

[dschwartznyc]

A couple of questions:

• Will the template address the minimum criteria and artifacts (documentation, test cases, et al) for PR approval?
• Under the new operating model, what is the PR approval process? Assuming that it varies according to the urgency and extent of the change, how many of which maintainers must have approved the PR for it to be considered for promotion? Do those approvals happen in advance or at the CRWG? Is there a recourse other than resubmitting if a PR is rejected?

[tomhealey-icma]

@dschwartznyc - I would hope the template can implement some of the rules and guidelines that can help streamline the PR process.

Under the new operating model, what is the PR approval process? Assuming that it varies according to the urgency and extent of the change, how many of which maintainers must have approved the PR for it to be considered for promotion?

The approval process is described in Ian's attachments posted at the top of the issue. Approval is not dependent on a CRWG meeting.

Do those approvals happen in advance or at the CRWG?
When a PR meets the necessary criteria and approvals it will be merged into the master for the next release.

Is there a recourse other than resubmitting if a PR is rejected?
That depends on why it was rejected and maintainers should be explicit as to why it's being rejected, and rejections should be supported by design guidelines. Some examples would be:
1. If maintainers agreed with the need and approach but requested specific changes of names, definitions or model structure, because it does not meet design guidelines then a contributor can make the changes and update the PR. I'm not sure if that's what you meant by resubmitting. If there are a lot of changes the PR can get a little messy so it may be better to close it and create a new one.
2. If maintainers disagree with the basic need or approach then it's up to the contributor to ask for a review at the CRWG or better a separate meeting to present their case or possibly an escalation to the TAs to discuss.

If anyone else would like to comment please do so.

[lolabeis]

@tomhealey-icma About 5. Documentation can be submitted in a separate PR after the model and code change PR.

I recommend making it a requirement to have the doc submitted together with the model and code change. Otherwise what mechanism do you have to ensure that the doc change does happen in the end? That's not good software development practice. The CDM has suffered from inconsistency between the model and the doc in the past, so safe-guards were put in place (checking doc snippets vs actual code) to prevent that from happening in future.

[dschwartznyc]

@tomhealey-icma About 5. Documentation can be submitted in a separate PR after the model and code change PR.

I recommend making it a requirement to have the doc submitted together with the model and code change. Otherwise what mechanism do you have to ensure that the doc change does happen in the end? That's not good software development practice. The CDM has suffered from inconsistency between the model and the doc in the past, so safe-guards were put in place (checking doc snippets vs actual code) to prevent that from happening in future.

Second this view. A PR should be complete at the time it is made. The checklist should include documentation, comprehensive unit tests where appropriate, et al and the code, if any, should have to pass scans. This is good hygiene, especially for an open-source project. If this is not a rigorous requirement, it will be a process of forever chasing missing artifacts.

[dschwartznyc]

Suggest a slight modification to the labels for the Target. Believe that the intent for the label is to distnguish whether the PR is for the current "Production" version of CDM or the next major version, With that in mind, to ensure that there's no confusion on intent, perhaps "Next" should be used instead of "Development"

Additionally, the suggestion that each PR be pre-approved by a WG needs to be withdrawn as that would only make sense if those groups existed for each major domain. The implications of the resulting bottleneck should be part of the broader process review discussion.

[lolabeis]

@tomhealey-icma @iansloyan @chrisisla I suggest we can close this issue now

[tomhealey-icma]

Before closing this issue I think we should agree on the final decisions:

1. An issue must be raised for model changes and other major changes prior to the PR and linked to the PR. PR template?
2. Proposed changes must follow the design guidelines.
3. PRs must have four tags: Category, Criticality, Complexity, Target.
4. Any and all maintainers have the right to comment on PRs.
5. Contributors should request pre-PR meetings with maintainers prior to or coincident with submissions.
6. A minimum of 1 approver is required for minor changes and 2 approvers for complex changes.
7. Release.md file is required with all PRs.

[chrisisla]

Agree with all 7 points @tomhealey-icma

For point 3 I think we need to consider whether we can change the github permissions so that the contributor can add the labels to their PR. I also don't think we need to add labels to the Issues, just the PRs themselves.

For point 5 I would suggest that a pre-PR meeting is not always required, it depends on the complexity and scope of the change (otherwise the maintainers could end up getting swamped with meeting requests).

Off the back of point 5 we also need to make sure that we have an accessible list of maintainers on github/cdm.finos.org and how to contact them.

[lolabeis]

We also need a point 8 (or add to point 7) that the PR should include all relevant documentation changes - although those may be provided later during the review process once changes are settled.

Re point 7:

An alternative to requesting to update the release.md file is to join the release notes as a PR comment, including all the relevant .md formatting. Github uses .md natively so the comment should render nicely.

Reason is that the requirement to update the RELEASE.md file is a legacy of how the CDM release process evolved over time when it was still close-source. We may want to get rid of it in future. In the meantime while we're still using it, if contributors provide a comment formatted in .md, maintainers can easily copy/paste that in the RELEASE.md file. It means we won't need to change our contribution process when we get rid of it.

We're already using that process (requesting to provide the release notes as comment) for other projects like DRR where we don't rely on a separate RELEASE.md file.

[lolabeis]

I suggest moving this discussion to being... a Discussion in GitHub, before we close it - Any objection?

[dschwartznyc]

Echoing @lolabeis's comment WRT documentation - it should be a requirement that a PR has all relevant documentation of the change and all required test cases prior to its acceptance/merge.

Also, to ensure that review can be arranged, the list of Contribution WG Maintainers needs to be publicly available.

[tomhealey-icma]

I will concur with the documentation requirement. Are there any other comments?

[tomhealey-icma]

Follow up from last meeting:

1. Modify approvals to require one maintainer from each TA.
2. Dev releases will can be run on any approved and merged PR.
3. Finalize agreement on monthly minor releases. Releases will be automated with pre-set dates.
4. Finalize agreement on annual production release.
5. Setup test repository and build workflows.
   FINOS - CDM Build and Release Proposal.pptx

[chrisisla]

On point 1 I don't believe it was agreed that every PR would need approval by all 3TAs. My understanding was that you were going to investigate how we could ensure that a PR was approved by maintainers from different realms e.g. a PR contributed by ISLA could not be merged if it was solely approved by ISLA/securities lending focused maintainers.

On this subject it is also worth bearing in mind that maintainers may not work for an association.

On point 2 can you clarify if dev releases "will" be run or "can" be run on any merged PR? If its the former then we'll have a 1 to 1 relationship between PRs and dev releases, is this the intention?

On points 3 and 4 I don't believe that we had consensus on automatically providing minor releases on pre-set dates. These two points still need to be ironed out but from the call I believe it sounded more like people were in favour of having target dates, but still be flexible enough to have more or less releases as required.

[dschwartznyc]

Adding to @chrisisla's comments regarding the scheduling of releases - a planned calendar is helpful but at this stage, it would seem that there is a real need for flexibility particularly when it comes to Major releases.

[lolabeis]

Closing this discussion, which appears superseded in all aspects by some of the components being discussed in #2774.

The rest looks all agreed.