Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

CONTRIBUTING.md: Add design reviews section #1116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomereli
Copy link
Collaborator

@tomereli tomereli commented Apr 7, 2020

New big features need to be documented properly.
Adding a design review (DR) as a requirement for every new feature will
make sure that the contributor gets the proper guidance from the more
experienced prplMesh maintainers / contributors. It will save us from
merging bad code since "its already there", and allow for better
quality.
Add a section which explains the requirement for design reviews to the
CONTRIBUTING.md file.

New big features need to be documented properly.
Adding a design review (DR) as a requirement for every new feature will
make sure that the contributor gets the proper guidance from the more
experienced prplMesh maintainers / contributors. It will save us from
merging bad code since "its already there", and allow for better
quality.
Add a section which explains the requirement for design reviews to the
CONTRIBUTING.md file.

Signed-off-by: Tomer Eliyahu <[email protected]>
@tomereli tomereli force-pushed the feature/contributing-design-reviews branch from df63a1d to 10246cd Compare April 8, 2020 09:49
Copy link
Collaborator

@arnout arnout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't think it's very useful to have DR files that describe changes in the repository. Rather, the files in the repository should describe the current situation. Therefore, I think the following workflow is more useful:

  1. If it doesn't exist yet, add a document + scenario diagram that describes the feature to the documentation folder. For historical stuff, there is no need to describe the rationale behind this design.
  2. Update the design document and scenario diagrams. Make sure that the design document includes a description of the feature (= requirements), a design for the feature (= scenarios and class diagrams), a design analysis (= different options with pros and cons, rationale of the chosen solution).
  3. Implement the design.

The diff between 1 and 2 nicely shows the proposed solution. If 3 is included in the same PR, we always have documentation that is up to date with the actual state. But even if 3 is split over several PRs, it's OK I think.

I've asked @VladyslavTupikin to take that approach in #1121 so we can evaluate if it's a good idea.

Every new feature / big change should be accompanied with a design review (DR).
It makes sure that the contributor gets proper guidance from more experienced prplMesh maintainers / contributors, prevents merging bad code "since its already there", and in general allows for better quality.

**Do not start working on a feature before a DR is approved by the community**. This will lead to potentially wasted effort, since you might go in the wrong direction, or in a worse scenario - "bad" code getting into our codebase, which we would really like to avoid.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONTRIBUTING.md tells you to use one line per sentence.

Suggested change
**Do not start working on a feature before a DR is approved by the community**. This will lead to potentially wasted effort, since you might go in the wrong direction, or in a worse scenario - "bad" code getting into our codebase, which we would really like to avoid.
**Do not start working on a feature before a DR is approved by the community**. This will lead to potentially wasted effort, since you might go in the wrong direction, or in a worse scenario - "bad" code getting into our codebase, which we would really like to avoid.

**Do not start working on a feature before a DR is approved by the community**. This will lead to potentially wasted effort, since you might go in the wrong direction, or in a worse scenario - "bad" code getting into our codebase, which we would really like to avoid.
> Tip - if you are not sure if a certain feature / bug fix requires a DR - ask first in #prplmesh-dev channel in slack.

The DR document shall be written in Markdown (.md) format, and will be the first commit in the Pull Request, under the documentation folder. For example, a DR which changes the autoconfiguration flow should be added to `documentation/flows/autoconfig`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The DR document shall be written in Markdown (.md) format, and will be the first commit in the Pull Request, under the documentation folder. For example, a DR which changes the autoconfiguration flow should be added to `documentation/flows/autoconfig`.
The DR document shall be written in Markdown (.md) format, and will be the first commit in the Pull Request, under the documentation folder.
For example, a DR which changes the autoconfiguration flow should be added to `documentation/flows/autoconfig`.

etc. etc.

> In order to support non-prplmesh controllers and eventually pass test 4.7.7 in the agent certification, the vendor specific flow will be replaced with the standard EasyMesh flows. This will be done in stages, as will be explained in the detailed design section, by automatically start monitoring connected clients when the controller is not prplmesh, and adding a non-vendor specific exit and entry points to the station measurement EasyMesh flow, finally replacing the current vendor specific altogether.

The detailed description section should dive into details and must include UML diagram\[s\] of the current prplMesh state, and the proposed solution\[s\].
> Tip - use [plantuml](https://plantuml.com/) format to describe UML diagrams.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> Tip - use [plantuml](https://plantuml.com/) format to describe UML diagrams.
> Tip - use [plantuml](https://plantuml.com/) format to describe UML diagrams.
> Put the uml files and the png files generated from it in documentation/images/plantuml, and refer to the png files from the markdown document, e.g. `![station measurements scenario diagram](../images/plantuml/sta_measurements_scenario.iuml)`.

Every new feature / big change should be accompanied with a design review (DR).
It makes sure that the contributor gets proper guidance from more experienced prplMesh maintainers / contributors, prevents merging bad code "since its already there", and in general allows for better quality.

**Do not start working on a feature before a DR is approved by the community**. This will lead to potentially wasted effort, since you might go in the wrong direction, or in a worse scenario - "bad" code getting into our codebase, which we would really like to avoid.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is what does it mean "not to start work". For a design review you need a design document. Sometimes it is a good idea to write some code, sketch your ideas inside the code to get the feeling of what should be done.
Maybe a better way would be phrase the sentence something like: "prepare to throw your entire code changes that you did before the DR" - meaning, code changes might be accepted only after a DR.

The DR document shall be written in Markdown (.md) format, and will be the first commit in the Pull Request, under the documentation folder. For example, a DR which changes the autoconfiguration flow should be added to `documentation/flows/autoconfig`.

A good DR document normally includes an introduction, detailed description and references sections.
The introduction section should give a first reader an overview of the feature. Think of it as an "abstract" of a scientific paper. It should summarize, in 2-4 paragraphs, the major aspects of the entire feature, the problem it aims to solve, current state analysis of the feature in prplmesh, and a brief summary of the proposed solution\[s\].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The introduction section should give a first reader an overview of the feature. Think of it as an "abstract" of a scientific paper. It should summarize, in 2-4 paragraphs, the major aspects of the entire feature, the problem it aims to solve, current state analysis of the feature in prplmesh, and a brief summary of the proposed solution\[s\].
The introduction section should give the reader an overview of the feature. Think of it as an "abstract" of a scientific paper. It should summarize, in 2-4 paragraphs, the major aspects of the entire feature, the problem it aims to solve, current state analysis of the feature in prplmesh, and a brief summary of the proposed solution\[s\].

@ghost ghost added this to the M3 - Workable product milestone Jun 17, 2020
@ghost ghost assigned arnout Jul 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants