As we all know, the MR robot is not only noisy, but also doesn’t always give the best advice. And usually we cut it some slack because it’s just a robot and is therefore blameless for its actions.
I have three suggestions that might make the robot more helpful, and would be very thankful to whoever implements them.
The “Do: reformat” could add a newline character to files that don’t have one, e.g. “sed -i -e '$a\' <filename>” before or after “clang-format”.
It could also be helpful if “Do: reformat” provided some cut-and-pastable commands for fetching the reformat, like the way gitlab gives commands for checking out a topic branch. New contributors to VTK are, unfortunately, likely to do “git pull” which results in a mess of conflicts and frustration.
Merge commits in MRs can potentially bring in unexpected code, if not caught before the MR itself is merged. At the very least, merge commits should cause a robot warning, and at best, MR’s with merge requests should be rejected (with certain people able to override for the sake of the third-party libraries).
We actually require this for all files. An “eol” formatter in this repo would allow us to enable such formatting for all files (not just clang-format-seen files).
We can do this for SetupForDevelopment.sh, but not in general (as we don’t know what the remote is named in general). I suppose we could just dump the URL and use FETCH_HEAD though.
There is a check that bans merge commits. However, they are necessary for backport MRs with conflicts and third-party updates. What I can look at is finding “twisted” merges where a first-parent history commit shows up as a non-first-parent. There is an issue to find and ban these.
I think we should improve the robot so that minimal human intervention is needed and there is less frustration for contributors.
Here is what I envision.
MR Created
Thanks for creating a MR for project, it looks like you did not run SetupForDevelopement.sh before creating this MR. please read develop.md and then run the script and push again to your branch.
- commands to do that
Contributor does that and force push
It looks like you created a MR from your master branch. Please push to a new branch and change the targets of this MR ... (more info on how to do that)
Contributor does it and robot run again
Formatting check ... please run Do: reformat and then reset your local repository (with example commands)
Contributor does that
Test check: Please run the test by using Do: test or manually running the tests in the pipelines tab. You can see the results in the cdash link. More info about reading test results on develop.md
Contributor does that, CI is green
Please request a review from a developer, I will fetch one for you: @mwestphal PTAL.
I know, maybe this is too much but at this current state of the robot, I always need to explain to contributors how to work. This should be automated as much as possible and develop.md should have at least a TLDR section to explain to contributors what they need to do.
Let’s first get the docs written up. Once we have words there, we can have the robot learn how to do that. Some of this is covered by this issue (though adding details from here to there would be good to not lose them).
Other bits just seem…chatty. Solutions also exist:
branch named master: use Topic-rename in the description to change the name the robot uses for merging purposes
reformatting…this is a check and the part that comments only gets back a “fail”; there’s not really structured information there to know why anything in particular is failing (it’s why the instructions are part of the check configuration)
the robot doesn’t currently listen to pipeline events (though I think it might receive them, they are ignored), so knowing when “CI is green” is not possible without polling (which we try not to do).