Add support for comments in secrets files #139
Conversation
This test currently fails because Vaultenv drops the second secret. I need to fix that, but let's add the test first.
Then when one fails, it will print only the failing example, not the list with all the results.
I want to make sure that touching the parser doesn't break things.
There is a place for true parsers, but the Megaparsec one here was fragile. I wanted to add support for comment lines to it but it was not so clear where, and the current parser had this pretty dangerous failure mode of ignoring half the input. Instead of trying to deal with that, for Vaultenv's secrets file format, it is so simple, just splitting on lines and then splitting on = and # should be fine.
| goldenTestContents <- Dir.listDirectory "test/golden" | ||
| let goldenTestFiles = map ("test/golden/" <>) goldenTestContents | ||
| parseResults <- mapM SecretsFile.readSecretsFile goldenTestFiles | ||
| parseResults `shouldSatisfy` (all isRight) |
There was a problem hiding this comment.
I changed this to have one it per file, so that when the tests fail, it only prints the failing file, instead of a big list with and “all isRight is false for this big list, good luck finding the offending one!”.
|
(NB, I didn't read the diff or try out the patch, I am just basing this off of your PR description)
If I recall correctly, this was also the approach I took in one of the first versions of Vaultenv. Then at some point we had a production outage due to some whitespace in a secrets file, which caused us to move to the parser-combinator based approach that was a bit more lenient with whitespace. |
|
Thanks for the PR @ruuda and thanks for the input @duijf! The production outage chronicled by @duijf seems to be a good argument to keep the megaparsec library. Along with it being a breaking change could mean that an update of Perhaps we can take another look at the challenge of implementing the comment parser in Megaparsec. The rest of the PR looks great. |
Are we perhaps missing a |
Motivation
I shot myself in my foot because of this line in the readme:
The comment is about
vaultenv.conf, not about the secrets file, so it’s entirely my own fault. Still, what happens when you mistakenly think that secrets files support comments, and you feed Vaultenv this file?Then Vaultenv will parse up to the comment, and silently drop the
BAR=definition.Changes
There was a Megaparsec parser but I found it difficult to introduce comment support. I think using a parser for this creates more confusion (and danger, as it dropped half my input) than a simple split-on-lines, split-on-
=, split-on-#. So I rewrote it without Megaparsec. It is now 100 lines smaller and supports comments.This makes some breaking changes: the version line must be
VERSION 2, other whitespace is no longer allowed. I think that’s acceptable but if you prefer to keep it compatible it’s also not too difficult. Either way if you writeVERSION 2that will fail to parse and not silently result in an unexpected secrets list. Also it is now a bit more lax about what path names and key names it accepts.