...
- Define the pull request review process for Indy Plenum/Node
- Should define the process, including how we handle exceptions (emergency fixes shouldn't be blocked, but would require notification)
- What is important in a good review?
- PR checklist items from Evernym team:
- has a link to the issue in Jira
- fix is correct according to requirements and the according to Plan of Attack (PoA) as shown in the Jira ticket and approved by an architect
- covered by tests (implementation is TDD style: both unit and integration tests)
- plan for documentation changes in the same issue, or links to other issues for documentation when it is a large effort (diagram creation)
- follows good design patterns
- follows https://github.com/hyperledger/indy-node/blob/master/docs/source/write-code-guideline.md
- multiple small pull requests are better than a single huge pull request
- PoA helps decomposition of work
- Documentation can be a separate PR for the same ticket
- Good review feedback
- Don't merge until the problem is fixed.
- But can merge if the suggestions are all a matter-of-taste, or smaller items that can be done in a follow-up pull request.
- Try to provide specific suggestions on how to respond to the feedback as a list or code-sample.
- PR checklist items from Evernym team:
- Proposed process for cross-organization PR reviews (by Evernym team):
- All Pull Requests can be reviewed by non-Evernym team members
- Evernym team members will also do internal review in addition to external one
- All interested parties are notified when a PR is sent
- If a person wants to do an external review, he or she puts a comment or tag. This needs to be done in X hours.
- Once a reviewer put a "want-to-review" tag, he or she need to finish review in Y hours
- If no one wants to review a PR in X hours, or review is not finished in Y hours, we can do our internal review and merge the PR
- An external review can be done against closed PRs as well, and Evernym team will process all findings ASAP
- We may merge a PR with internal review only in case of urgency (critical fixes, release preparation etc.), and with reporting afterwards
- Items to be defined with the Community:
- A timeframe to start
- Would like to start the process in November if possible
- A timeframe for external review (X):
- X=
- 18 hours, Y=
- 24 hours
- X being 18 hours would mean that the team doesn't usually merge pull requests in the same day they are created, but can be merged at the start of the next business day
- Y being 24 hours means that the review will complete the review within a day of saying that he wants to do the review. Some reviews might take longer, if the reviewer asks for additional time to complete the complicated review.
- What projects it should affect?
- Only Node?
-- Node is preferred for now (disrupt one project at a time)
- Most fixes at the moment are in Plenum, so can include them as well
- We are not proposing SDK as it will be split to Aries in any case.
- Node is preferred for now (disrupt one project at a time)
- Who is going to commit to participate in this process?
- Ken: one review a week, at a minimum
- Kiva: could probably start in a month-or-so
- How to notify:
- GitHub list of reviewers
- Rocket chat #indy-maintainers can help us highlight priority reviews
- A timeframe to start
- Improvements to the proposal
- Alex's team labels PRs that they think would most benefit from reviews.
- We can schedule appointments for reviewing specific PRs together.
Future Calls
- Migration of Indy-SDK to Aries-Core
- Requirements question: IS-1099, should we allow duplicate credentials from the same issuer?
- Non-secrets in the Indy Wallet
- Cam is working on pluggable crypto. The wallet shouldn't decide what encryption you should be using.
- Use cases where we would want to move keys between wallets
- Moving the link secret / credential data from one device to another (synchronized storage).
- Debug use cases
- Richard's hit other uses cases that were better solved with DID Doc, pre-signing, signing API.
- Work-around with the web-crypto API
...