Igor Posted March 16, 2022 Posted March 16, 2022 (edited) 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 March 17, 2022 by lanefu clarification of inline comment resolution vs change requests 2
lanefu Posted March 17, 2022 Posted March 17, 2022 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. 1
balbes150 Posted March 17, 2022 Posted March 17, 2022 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.
Werner Posted March 17, 2022 Posted March 17, 2022 Even though I do not often send code or changes I think the suggested stuff makes sense. 1
lanefu Posted March 18, 2022 Posted March 18, 2022 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.
Igor Posted March 18, 2022 Author Posted March 18, 2022 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.
lanefu Posted March 19, 2022 Posted March 19, 2022 New rules have been applied. Will wait 1 week before requiring code signing... so get your code signing working folks @lanefuI'm talking to you, buddy
Igor Posted March 21, 2022 Author Posted March 21, 2022 Review needed. https://github.com/armbian/build/pull/3552 1
Igor Posted March 21, 2022 Author Posted March 21, 2022 https://github.com/armbian/build/pull/3548 Since we have more then one beta server we need to sync them before making images.
Igor Posted March 25, 2022 Author Posted March 25, 2022 A few of ready to merge but needs someone attantion: Build images on PR if label is set to "Desktop" Fixing deprecation notice for our primary key Refactor all PPA sources due to apt-key deprecation
Igor Posted March 26, 2022 Author Posted March 26, 2022 More reviews are waiting https://github.com/armbian/build/pulls 👀
Igor Posted March 31, 2022 Author Posted March 31, 2022 Bug fix https://github.com/armbian/build/pull/3616
Igor Posted April 11, 2022 Author Posted April 11, 2022 More pull request review needed https://github.com/armbian/build/pulls
Igor Posted May 2, 2022 Author Posted May 2, 2022 @private/contributor Unifying TAGS as much as possible - as low count / universal as possible has been enforced to GitHub merge requests and issues. From now on, we have only 12 labels: 2
Recommended Posts