Helping to review pull requests

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
openBrain
Posts: 5209
Joined: Fri Nov 09, 2018 5:38 pm

Re: Helping to review pull requests

Postby openBrain » Tue Jan 05, 2021 3:48 pm

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
Posts: 30821
Joined: Tue Mar 17, 2015 9:14 am

Re: Helping to review pull requests

Postby chrisb » Tue Jan 05, 2021 5:56 pm

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.
You need at least FreeCAD 0.19.23300 to edit my current sketches.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
hyarion
Posts: 54
Joined: Fri Jun 26, 2020 6:08 pm

Re: Helping to review pull requests

Postby hyarion » 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?

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
Site Admin
Posts: 12147
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels, Belgium
Contact:

Re: Helping to review pull requests

Postby yorik » 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. 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: 49
Joined: Mon Dec 21, 2020 4:22 pm
Location: Greece

Re: Helping to review pull requests

Postby voskos » Thu Jan 07, 2021 12:55 pm

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: 236
Joined: Fri Sep 18, 2020 5:40 pm

Re: Helping to review pull requests

Postby davidosterberg » Thu Jan 07, 2021 1:11 pm

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
Posts: 5209
Joined: Fri Nov 09, 2018 5:38 pm

Re: Helping to review pull requests

Postby openBrain » Thu Jan 07, 2021 2:02 pm

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: 37
Joined: Wed Dec 30, 2020 2:16 am

Re: Helping to review pull requests

Postby DrInfiniteExplorer » 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.
User avatar
Kunda1
Posts: 9222
Joined: Thu Jan 05, 2017 9:03 pm

Re: Helping to review pull requests

Postby Kunda1 » Thu Jan 07, 2021 9:36 pm

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
Want to contribute back to FC? Checkout:
#lowhangingfruit | Use the Source, Luke. | How to Help FreeCAD | How to report FC bugs and features
GeneFC
Posts: 1805
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: Helping to review pull requests

Postby GeneFC » Fri Jan 08, 2021 4:34 pm

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