bork icon indicating copy to clipboard operation
bork copied to clipboard

sketch on fixing the quote problem

Open mattly opened this issue 6 years ago • 2 comments

DO NOT MERGE. Actually I disabled merges that don't pass CI, and this sure as hell won't.

So this is a stab at #87 by @frdmn whereby bake isn't handling quoted arguments well. And, I knew I was doing something hacky with bake, but I really had no idea just how hacky. Short of reworking all the types to turn bake ls -la $dir into bake "ls -la $dir", this seems to be the next best thing: escaping quotes on every item in the argument list to bake.

The problem is bake is so central to everything, all for the sake of having nice, stubbable tests, whereby I could test the logic for f.e. apt on my mac, or brew on a linux CI server, without resorting to VMs and whatnot. So now pretty much all of the tests are going to fail and need to have their respond_to and baking_output stuff changed.

Since I'm not even sure this will have the desired effect without breaking everything (it seems to work ok locally) I'd like to get some eyes on it before going to the effort of updating all the tests.

mattly avatar Jan 28 '18 08:01 mattly

I should note that this only solves the problem with bake, it doesn't solve the problem with ok. 😿

mattly avatar Jan 28 '18 08:01 mattly

I fixed ok, but now it comes down to making sure everything is escaped properly in each of the types. I did that for git and github and updated the tests.

This is the sort of thing that makes me question the whole project to begin with, because Bash does all this magic with string quoting and argument positions with rules that are easy to trip over. What's been happening is:

  1. bork script calls ok github "my dotfiles" mattly/dotfiles and ok was calling github status my dotfiles mattly/dotfiles --ssh.
  2. Once I fixed that so it was calling github status 'my dotfiles' 'mattly/dotfiles' '--ssh', then github was calling ./git status my dotfiles [email protected]:mattly/dotfiles.git.
  3. so I had to fix that to call ./git status 'my dotfiles' '[email protected]:mattly/dotfiles.git but then git was calling [ ! -d my dotfiles ] so I had to fix everywhere in git where $target_dir was used so it was quoted properly.
  4. Next we get into the tests and how "respond_to" and "baked_output" work. For the latter, I think it's much clearer, you have to explicitly go looking for f.e. " 'git' 'status' '-uno' '-b' '--porcelain'" so you know things were quoted directly. And you have to do the same thing with the mocks and respond_to. It's ugly, but workable.

It's the sort of thing that makes me think I should try to rework how bork works to remove a layer or two of indirection, but then if I did that bork manifests wouldn't have the same sort of repurposability they have with status/satisfy/etc. Or maybe push the "data" of the manifests off into an actual data file, or its own format, but then you couldn't have that inline logic you so often need. May as well just use ansible, then. If I had the inclination I'd eliminate bake and setup some sort of super-magic multi-environment testing harness with some sort of virtualization/container tech, but I don't. If someone else wants to tackle that, let me know.

Then problem then becomes, how do we ensure people adhere to the stuff necessary to maintain good quoting all the way through? And then I get tempted to port quickcheck to Bash, so that for example, you don't test "directory works with 'mydir' and 'my dir'", but rather "directory works with anything that resembles a valid directory name", which I'm not even really sure how to begin going about. Then I remember I have lots of other things I could be doing with my spare time that are much higher on the "want to" list. I mean maybe you could use lists and for as well. That might be worth a shot.

Anyway, the problem is large but not insurmountable, but it's a lot of work and could cause some breakage with existing scripts or behaviors.

mattly avatar Jan 28 '18 22:01 mattly