Jump to content

Git - code quality improvement attempt


 Share

Recommended Posts

As our projects grows in size and complexity it is getting more and more important to add additional measures to improve code quality. Code is added almost daily and therefore we have to do what is possible to improve chances that things doesn't break or have obvious bugs.

 

Therefore I propose to enable additional conditions applied to the build framework master branch:

 

- code must be submitted via a pull request
- pull requests require at least one review and approval from another individual

- all changes open requests resolved before merge.

- in-line conversation on merge request has to be resolved before merge
- commits pushed to master must have verified signatures,
- conversation on merge request has to be resolved before merging.

 

@private/contributor


Most of the things we already use, so new thing is mandatory review - counts from anyone except from the one that is sending a pull request.
 

Speak up if you find those measures too draconian.

Also I would like to point out another change related to our label system. It seems too complicated, some tags were never used and not many people are using them. So limiting them down significantly - comments are welcome here https://armbian.atlassian.net/browse/AR-959

Edited by lanefu
clarification of inline comment resolution vs change requests
Link to comment
Share on other sites

When discussing a problem make sure to provide full logs!

On 3/16/2022 at 4:13 AM, Igor said:

Most of the things we already use, so new thing is mandatory review - counts from anyone except from the one that is sending a pull request.
 

Speak up if you find those measures too draconian.

 

@IgorI support this 1000% This is common practice is large projects in open source and enterprise.     I definitely think we're ready to add this process.   It's a great sign that our project is growing more mature.

 

Lets strongly encourage that _anybody_ can click to suggest a reviewer so that the PR and discussion can get attention quickly.      Great chance for all the devs to learn more about each other.

 

Link to comment
Share on other sites

Maybe I don't know something, but it seems to me. previously, this was the case, a PR was sent and before accepting it, it was considered by interested developers. It may be worth defining categories (groups) of developers in the areas to which the request for approval is sent. if the PR affects the basic things for the whole group (kernel, u-boot, settings, etc.). For example, the group Rockchip64, Rockchip32, Allwinnre32\64, etc.

Link to comment
Share on other sites

22 hours ago, balbes150 said:

Maybe I don't know something, but it seems to me. previously, this was the case, a PR was sent and before accepting it, it was considered by interested developers. It may be worth defining categories (groups) of developers in the areas to which the request for approval is sent. if the PR affects the basic things for the whole group (kernel, u-boot, settings, etc.). For example, the group Rockchip64, Rockchip32, Allwinnre32\64, etc.

 

It was often the case but not always.    Didn't happen 100% of the time.   

 

We're in a spot where we really need to be more thorough.

Link to comment
Share on other sites

22 hours ago, balbes150 said:

Maybe I don't know something, but it seems to me. previously, this was the case, a PR was sent and before accepting it, it was considered by interested developers. It may be worth defining categories (groups) of developers in the areas to which the request for approval is sent. if the PR affects the basic things for the whole group (kernel, u-boot, settings, etc.). For example, the group Rockchip64, Rockchip32, Allwinnre32\64, etc.


Lets say it is desirable that someone from specific group checks the code, but in most cases this review should at least check for most obvious troubles any developer can spot, perhaps propose changing approaches  in scripting, coding style. I don't expect Rockchip / Allwinner / * specialist will be able to see that some patch breaks some functionality. This is a job of automated test routines on hardware. Which is very much WIP but that's what we can afford ATM.

 

If we engage more people to view the code before merge, we made a step forward. And that counts!

 

It doesn't need to be a regular team member. Anyone with a Github account and some time and interest to help us code better is welcome to comment and slow the process of merging. I think we also need to create a protocol for emergency merge / corner cases.

Link to comment
Share on other sites

  • Igor pinned this topic
  • Igor unpinned this topic
 Share

×
×
  • Create New...