template doesn't fail if CSV file contains git conflict markers

[paulmillar]

We're using a CSV file as input to a robot pipeline (via the template command) that is generating various semantic artefacts. This mostly works very well. The CSV file is stored in git repo, hosted on GitHub.

In the project, we use a script that builds the semantic artefacts (among other tasks). This script is containerised and run automatically when changes are committed to the git on GitHub, as part of a CI/CD pipeline. If robot command fails, the CI/CD pipeline fails, which GitHub shows graphically, against that commit. Therefore, we can use the failure of the robot command to identify problems.

Unfortunately, while working through a git merge conflict, someone mistakenly committed the merge conflict markers. For reference, the git conflict markers are lines that start with seven identical characters (<, = or >). The lines starting < or > contain additional information.

Here are example conflict marker lines:

<<<<<<< HEAD

and

=======

and

>>>>>>> c15b766 (blah blah blah...)

(where blah blah blah... is a placeholder text I've added, which would normally be the one-line summary of the corresponding commit)

Currently these lines do not cause the robot template command to fail. Therefore, although the commit introduced a problem, our CI/CD pipeline didn't detect this. It was only discovered from manually inspecting the CSV file (because of an unrelated problem).

It would be great if robot template command were to fail if the CSV file contained such git conflict markers.

For reference, we're using the template command with the following options --template $input --prefix "PaNET: http://purl.org/pan-science/PaNET/". The output from template is fed into further robot commands (reason, annotate, measure, etc), so the complete robot command is much longer.

[matentzn]

Thank you @paulmillar! I encountered the problem too, but I decided (in my head, and for myself) that ROBOT is not the right place to address this. Not because I dont think that in 99% of all conceivable cases this is a legitimate error (it is!), but because I am just not sure whether we should pepper the code with such exceptions that are not fundamentally wrong (you can imagine a perfectly fine TSV with these markers).

I am not the main ROBOT maintainer, so I will of course defer to @jamesaoverton on this, but in my view, you can make panet CI/CD more robust (and faster!) by matching conflict markers with grep before ROBOT is even fired up. But - this is just my opinion, its a close call (i feel very weakly here about my own sentiment and could totally support the opposite conclusion).

[paulmillar]

In the spirit of full disclosure and to help anyone else suffering from this problem, I updated our CI/CD pipeline to use the GItHub action olivernybroe/action-conflict-finder@v4.0. This isn't an endorsement of this approach or of this specific GitHub action, just to describe the approach we're trying.

I agree that one could argue that the git conflict markers are valid input for a CSV file: they're "not fundamentally wrong".

However, please note that these lines lack comas. The middle line (=======) certainly doesn't contain any commas. The first line (<<<<<<< HEAD) is also unlikely to contain any commas (although I believe this would be possible). The final line could contain commas; in my case it didn't.

In any case, the number of commas is unlikely to match the number of columns, as defined in the CSV's header (first two lines). Therefore, these lines are incomplete. Either the "template" command ignores incomplete lines (in which case, an equality valid response would be to fail) or "template" assumes that absent cells are blank (in which case, an equally valid response would be to fail).

If it's desirable to allow comments in the CSV file then support could be added; for example, by ignoring any line where the first non-white character is #. Literal # would be supported by quoting the cell.

I could imagine that backwards compatibility is a concern, so a change would likely require an option to enable the change (e.g., a parsing option that accepts values strict and lax, where lax would be the default).

[jamesaoverton]

@paulmillar Thanks for the detailed report. I read it when you posted it last week, and I've been thinking about it since. I've decided that I want to keep the current behaviour, where ROBOT does its best to process anything you throw at it. My main reason is backwards compatibility, as you said.

I would be fine with adding a strict option, defaulting to off, that would check the template file and quit if it finds errors, before actually processing the template. It would have to allow for comments, as you suggested. I would not want to modify the main template processing code. This is not a priority for me, but I would help with a PR if anyone is interested.