Taking care of infrastructure: code review

Organisation
#1

Continuing the discussion from Donations for the foundation:
In this post I said that we are missing resources to maintain and improve the infrastructure on which the Tryton development is done.
For me the principal issue is that Rietveld our codereview tool is still based on Python 2. On the server side it is not yet an issue because the appengine of Google is supporting Python 2. On the developer side it may become an issue because upload.py is also only Python 2 and many distribution are already planning to remove Python 2 (for which support has ended).
We have at many times tried to find a replacement:

I would say that after having searched and tested many tools, I could not find an Open Source tools that could replace Rietveld out of the box.
I came to the conclusion that we must build our codereview tool based on our existing infrastructure.
So the idea is to have an application website (probably Python-Flask-Bootstrap) that is linked to our Roundup by sharing login, session etc. (maybe a subpath like bugs.tryton.org/review). This application will take from its URL parameter the id of a patch file in roundup and display it for review (using difflib). We may add some javascript to record and display comments par line. And finally an action to send by email the comment to the user watching the review.
To upload the patch, we could have a simple python script in tryton-env that send by email to bugs@tryton.org the mercurial diff of the repository to the proper issue. Of course the basic submission by the roundup web page will work also.
Finally we could trigger a build on drone.tryton.org through trypod.tryton.org, send failure to the author and retrieve the status as badge next to the patch.

Once the basis are there, we could continue to improve it with:

  • grouping patch under a history navigation
  • integrate review-bot
  • patch reformatting with proper commit/author header
  • early merge conflict

At B2CK we estimate that we could build the basis of such tool in 2 weeks of development.

#2

I’m clearly not an expert in the Tryton workflow tools, but when I have to build my own codereview tool because the current one is no longer supported by upstream, I would ask myself the question how future-proof the whole setup is.
It is just a question of time until the next ‘workaround’, and at a certain point in time you are stuck with an unmaintainable proprietary solution.
Maybe this is the point in time to think about a completely new set-up.

#3

hi, @ced did you test Review Board too?

#4

Yes Issue 2178: Replace Rietveld - Tryton issue tracker

#5

I think we should consider https://heptapod.net/

#6

Heptapod is a good news for Mercurial but it is still under development and not stable. It will require a massive migration of all the workflow and tools.
More over it does not support our design with sub-repository and GitLab is very resource consuming.

#7

This is exactly the same as having a custom solution. The main benefit of using heptapod is that it has a wider audience so we do not assume the full development (but we can contribute to it).
Furthermore, we can wait until it’s more stable and keep the current workflow until there.

Then we may consider using a mono-repository as we already discussed.

But it has a free hosted option for open source projects we may opt-in. So we do not need to worry on the resources.

Furthermore it has also many features like:

  • Integrated CI (for codereview also)
  • Possibility to propose changes directly from the web interface
  • Docker containter registry

And for me the main benefit is that uses a workflow which most of the developers are familiar. This lowers the learning curve to contribute to Tryton.

1 Like
#8

You are not considering the massive migration of all current tools.

Are you volunteering to make such merge? For now nobody ever made a testing proposal for this idea in 2 years.

I doubt they will still give us free resource when they will see our requirements.
But as long as nobody ask them, we can not know.

This is them same song as those who wanted us to move to GitHub.
Contribution from people who can not follow our guide lines are just added burden to the maintainer (aka me).

Is it really needed? For me the main blocker is that we are strict on the quality.

#9

Not sure which are all the tools that you talk about. It will be great if you can list so we can discuss what to do in each case.

I see some tools will be replaced by gitlab:

  • codereview will be done on gitlab
  • No more needed to use a script (upload.py) to propose reviews.
  • flake8 checking can be done on CI (review-bot won’t be needed anymore)
  • drone will be replaced by gitlab CI (trypod won’t be needed anymore)

As far as we continue using mercurial the maintainer work should not change so much. Probably the only diference is how changes are pushed but they can also be done using standard mercurial tools.

1 Like
#10

All of them, looks at the menu of gitlab you will see.

More bad/low quality contribution means more work.

#11

There is no need to migrate everything. We can just use gitlab for code hosting, codereview and CI. And continue using the same tools as we use now.

#12

This lowers the learning curve to contribute to Tryton.

Is it really needed? For me the main blocker is that we are strict on the quality.

More bad/low quality contribution means more work.

But not all are developers, what about those who write the documentation?

#13

Where are they? We already said that people can just send plain text and we will take care of formatting it.
For now we received 0 text.
Also you need to know deeply Tryton to write good and correct documentation.

#14

Here is one example shared on this forum.

Of course the quality should be improved to land as oficial tryton documentation, but if we give the user the right tools and the needed comments I do not doubt they will suceed by providing good documenation.

Deeply nowing Tryton does not mean knowing how to develop Tryton and the tools used.

#15

If we want to stay on Mercurial the best way to review is pre-commit reviews. Rietveld has the best usability of all tools I know, and there are not many tools around.
But Rietveld is no longer maintained. So we need to maintain at least its functionality by ourselves. This is a challenge, as we maintain already a lot of our infrastructure.

But IMHO all hassle we have, is because we use Mercurial in the first place.
We need pre-commit reviews only, because once a commit is done, it stays immutable in Mercurials commit history.
Other VCS like git have a different approach to immutability, which is not immutable per se. They use a Pull-Request workflow made of disposable dev-branches. And these VCS let the maintainers decide the quality of the commits and the history. Here the quality and consistence of the commit history depends on maintainer discipline.
There are many tools and web services like SCMs around the Pull-Request workflow and git. Once a pull request is “LGTM”, it can be squashed to one single commit and applied to the trunk.
Pull-Requests can be discussed line by line. And every iteration of the code evolution is visible in a new commit in the pull-request-dev-branch. IMHO it has a similar visibility and usability like the patch sets and the line comments in Rietveld.
The pull-request approach is very common to programmers nowadays.
But the git and pull-request-workflow is not for free: migration from mercurial and a new set of tools, routines and documentation is a challenge, too.

I constantly ask myself, which approach is better and more “future-prove”? What do you prefer?

2 Likes
#16

And also does not work when using as many repositories as Tryton does.

#17

What is the problem?

#18

Pull requests only work for a single git repository, but currently Tryton uses Rietveld with patches that affect several repositories.

1 Like
#19

I just found an interesting guide from mozilla explaining how they adapted phabricator review tool to suite its needs.

#20

I have setup a phabricator instance some weeks ago. It has mostly all a project needs.

With mercurial codereview is not working without a local commit. For me that is ok because on the next change I ‘strip’ the last commit and do a new local commit. But it is not a ‘pre-commit’ like stated on there page.
But in phabricator you have all the tools together (CI, review, issue tracker, user management 
 ) - but you need to explore them.