Helping to review pull requests

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Helping to review pull requests

Post by openBrain »

hyarion wrote: Tue Jan 05, 2021 9:59 am Ah, my bad. I now understand your definition of a greenhouse version. That sounds really cool!
The only down side I can see is that it would be possible to add malicious code that gets executed for unsuspecting users without proper review steps.

Only allowing branches from old contributors would make this less of a problem, but still a potential threat.

How about if we added a time limit as well so it only adds branches that’s been in review for x-days? That way there would be a window to find malicious code.
There always be a risk, but we can indeed try to minimize it. One solution is to let some days before merging. Another would be to have a bunch of trusted 'greenhouse mergers' that will just check the code doesn't own anything suspect. This is generally easier that full checking funtionality + coding style + side effects + ... :)
Anyway, maybe my proposal isn't the right one, but I really think we have to better value code contributions by offering PR authors a quick acknowledgment and feedback.

EDIT : also we can encourage users to run greenhouse version in a sandbox such as Firejail or Sandboxie.
Last edited by openBrain on Tue Jan 05, 2021 7:39 pm, edited 1 time in total.
chrisb
Veteran
Posts: 53933
Joined: Tue Mar 17, 2015 9:14 am

Re: Helping to review pull requests

Post by chrisb »

hyarion wrote: Tue Jan 05, 2021 9:59 am The only down side I can see is that it would be possible to add malicious code that gets executed for unsuspecting users without proper review steps.
As far as trusted users are involved, it would be a similar threat as if people compile a non trivial pull request themselves. I can imagine that people don't check each file for malicious code before compiling.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
hyarion
Posts: 139
Joined: Fri Jun 26, 2020 6:08 pm

Re: Helping to review pull requests

Post by hyarion »

This discussion has derailed from the main topic, how about splitting it around the start of the greenhouse post?

chrisb wrote: Tue Jan 05, 2021 5:56 pm As far as trusted users are involved, it would be a similar threat as if people compile a non trivial pull request themselves. I can imagine that people don't check each file for malicious code before compiling.
I see your point but I think theres a difference since this greenhouse version would be a composite of 50+ pull requests which has not been looked at. So it should at least be 50+ times the threat.

I would like to guard end users from accidentally install something that could, for example, remove all data on their system.
I don't know how we could do it though, maybe it's what openbrain suggested earlier, or maybe just a disclaimer.
I think it's a good discussion to have before releasing such versions though :)
User avatar
yorik
Founder
Posts: 13640
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels
Contact:

Re: Helping to review pull requests

Post by yorik »

I see the interest of the greenhouse idea, of course... But we tried that many times in the past (when we switched to GIT basically everybody started developing stuff in separate branches), the problem being that unless you build precompiled packages for all platforms, basically almost nobody will test your code. The same would happen here, we'd have a bleeding edge greenhouse branch, well, almost nobody will use it. You see that well with Realthunder's branch, people started using it when he started offering precompiled packages.

So we should get an automatic build system, right? That builds packages automatically. Obviously, YES! But we'd need that for the master branch too. But 1) we don't have an own server, which would be the best solution, and 2) More and more CI platforms such as Travis as putting limitations.

Automatic builds and better auto tests structure, I think those would be good goals for after 0.19 release, no?
User avatar
voskos
Posts: 67
Joined: Mon Dec 21, 2020 4:22 pm
Location: Greece

Re: Helping to review pull requests

Post by voskos »

yorik wrote: Thu Jan 07, 2021 11:29 am Automatic builds and better auto tests structure, I think those would be good goals for after 0.19 release, no?
I agree 100%
I write software for a living, as many others in this community do. I won't talk about test driven development, or other such stuff. I will say the only indication of correctness lies in the automation tests. Manual tests are very good at verifying the automation tests, but not the code. This code is hard to test, that is the code's fault, not the tests.
davidosterberg
Posts: 529
Joined: Fri Sep 18, 2020 5:40 pm

Re: Helping to review pull requests

Post by davidosterberg »

voskos wrote: Thu Jan 07, 2021 12:55 pm Manual tests are very good at verifying the automation tests, but not the code.
I agree with what you said. But the greenhouse would be good for UI/UX experiments. Do the users like the workflow, is it intuitive etc. And they could identify corner cases that could make it into the test suite. I think automated tests and the greenhouse would complement each other nicely.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Helping to review pull requests

Post by openBrain »

hyarion wrote: Tue Jan 05, 2021 7:17 pm This discussion has derailed from the main topic, how about splitting it around the start of the greenhouse post?
I don't think it has derailed too much. It's about making easier the life of those who want to review (especially functional tests). However splitting may be a good idea too. :)
yorik wrote: Thu Jan 07, 2021 11:29 am I see the interest of the greenhouse idea, of course... But we tried that many times in the past (when we switched to GIT basically everybody started developing stuff in separate branches), the problem being that unless you build precompiled packages for all platforms, basically almost nobody will test your code.
Another possibility would be to build the greenhouse version for only one platform (say Linux) and distribute it as a VM disk. ;)
Automatic builds and better auto tests structure, I think those would be good goals for after 0.19 release, no?
Depends when will happen the release. :P Maybe not too soon though to build the plan. What server resources do we need ? How much does it cost and can FC project afford it ? Do we have skilled (and volunteering) people to set up and maintain it ?
User avatar
DrInfiniteExplorer
Posts: 39
Joined: Wed Dec 30, 2020 2:16 am

Re: Helping to review pull requests

Post by DrInfiniteExplorer »

Well, if we're talking about formalizing and improving the PR review/merge workflow, then we could require that every PR contain an associated regression test. Of course exceptions are to be made when that makes no sense.

I'm pretty sure that it's possible to write tests that performs interactions with Qt from python, to verify that the interface behaves as it should.
User avatar
Kunda1
Veteran
Posts: 13434
Joined: Thu Jan 05, 2017 9:03 pm

Re: Helping to review pull requests

Post by Kunda1 »

DrInfiniteExplorer wrote: Thu Jan 07, 2021 4:35 pm Well, if we're talking about formalizing and improving the PR review/merge workflow, then we could require that every PR contain an associated regression test. Of course exceptions are to be made when that makes no sense.

I'm pretty sure that it's possible to write tests that performs interactions with Qt from python, to verify that the interface behaves as it should.
This sounds like a very good idea and helps root out regressions.
Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
'good first issues' | Open TODOs and FIXMEs | How to Help FreeCAD | How to report Bugs
GeneFC
Veteran
Posts: 5373
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: Helping to review pull requests

Post by GeneFC »

Kunda1 wrote: Thu Jan 07, 2021 9:36 pm This sounds like a very good idea and helps root out regressions.
May also help root out PRs. :o

Obviously regressions are not good, but adding an extra burden to creating PRs may be worse.

Gene
Post Reply