Metaswitch / floki

Launch containers to help build your software
MIT License
66 stars 21 forks source link

`floki` doesn't find `Dockerfile` when searching down directories #55

Closed rlupton20 closed 4 years ago

rlupton20 commented 4 years ago

If your directory tree looks like

| floki.yaml
| Dockerfile
| some_folder/

and you run floki in some_folder, then floki will search down the directory tree for floki.yaml, but if configured to use the Dockerfile for the image, it will fail to find the Dockerfile.

rlupton20 commented 4 years ago

@tomwatson1024 you might be interested in this, otherwise I can take it on. It should be an easy fix.

tomwatson1024 commented 4 years ago

I've had a quick look - I'll try and make a (quick) fix sometime today. I've got a slightly tangential question about the "right" way to do this that someone more familiar with Rust might be able to answer.

We use serde to deserialize the Image enum, which includes PathBufs (or Strings representing paths). It would be nice to be able to transform relative paths during deserialization to be either absolute or relative to the floki root. Patching them after the fact seems a bit unpleasant, as we have to a) mutate the structure and b) publically expose the members that require changing, as the Image is automatically deserialized as part of the Config struct.

What's the most idiomatic way to do this? I've noticed serde allows use of a custom deserializer function via serialize_with, but I can't see how to supply "extra" parameters, e.g. the floki root.

rlupton20g commented 4 years ago

No rush on this - no need to worry. I thought you might like to do the fix is all.

Usually I just use functions to do that kind of work. The Deserialize instance is just concerned with parsing the configuration file, so I don't think we should start to conflate it's purpose. It might take a bit of playing to find a way to express it with good taste (so to speak).

In general I think the way floki is structured follows the pattern

read things (cli, config, environment) -> combine and configure things -> run things

It's possible the Image enum conflates the first and middle steps a bit - I wouldn't be against breaking them up.

tomwatson1024 commented 4 years ago

No rush on this - no need to worry. I thought you might like to do the fix is all.

Yep, it's interesting enough that I wanted to take a look today 😛

On your second point - that does make sense. Perhaps the Image enum should just represent the config as read, then be passed into some free function, also taking the floki root, that then handles building the image. It seems essentially the same as a "member" function on Image, but maybe it better represents what we're after?

rlupton20g commented 4 years ago

Yeah - have a go and see. In general feel free to move things around to make them clearer. floki grew organically from a few lines of Python, so it's expected to rework things. Rust makes that nice anyway.

rlupton20 commented 4 years ago

@tomwatson1024 - I think we're done here - do you want to raise an issue to improve the testing?

tomwatson1024 commented 4 years ago

I've raised https://github.com/Metaswitch/floki/issues/58 to cover the testing improvements. Happy to close this now!

rlupton20g commented 4 years ago

Ace - I'll close this for now. Thanks for the fix!

rlupton20 commented 4 years ago

Fixed in #56