fixes #12730 - removing the fakeFS gem

[unorthodoxgeek]

No description provided.

[theforeman-bot]

There were the following issues with the commit message:

• 9ef4fee must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[domcleal]

Please add info about the "issues" on the ticket so we have it for future reference.

[domcleal]

Looking at the test that fakefs has been removed from, it looks like this will now hit the filesystem rather than being mocked, leaving this stray file behind in a top-level dir. It should probably be written to use a secure temp file if it can't be mocked.

[unorthodoxgeek]

@domcleal - now removing the files this test has created.

[domcleal]

Yeah, that's a bit messy and still leaves the test possibly interfering with development/production environments or vice-versa. It would be safer to replace fakefs with a temporary directory or full mocking of the File calls instead of interacting with the real Rails public path.

I think I'd prefer to see this run in a mktmpdir block and pass the temporary path into the method under test, with it defaulting to the prod directory for non-test usage. The file handle should probably be a regular Tempfile.

[unorthodoxgeek]

@domcleal - this is the mother of all nitpicks, those files do not interfere with anything on development, they can't do that. Why complicate this insignificant change for the remotest of remote possibilities of interference with a development environment?

I see no upside in the changes you suggested, while merging this ASAP releases foreman-docker from its current state of limbo.

[domcleal]

Tests shouldn't be writing to the filesystem like this, I stand by my comment. It should be correctly stubbed (as it was before) or moved to a temporary dir.

[unorthodoxgeek]

@domcleal - done. pointless, IMHO, but done.

if I'm already at it, the way we're saving the files seems like a VERY bad idea. I'd consider using something like https://github.com/carrierwaveuploader/carrierwave or similar libraries. In foreman instances which run across multiple hosts behind a Load Balancer, those files will exist on the host which first created them, but won't exist on other hosts, leading to round-robin 404 errors, which might be funny to say, but isn't funny to debug 😄

I'd consider adding a feature request on this, which will allow us to configure an S3 bucket or similar stuff for other cloud providers, so multi-host foreman instances can work (and also, single hosts which don't want to rely on the host's file system)

[domcleal]

This can also go straight into the temp_dir too now, FWIW.

Edit: saves having to delete it separately.

[domcleal]

Indeed, all good points - please do file it, it's worth doing.

[ohadlevy]

why not simply store the image on the db instead?

[domcleal]

This line's unused now the file creation was moved.

[unorthodoxgeek]

@domcleal - can we merge this?

[domcleal]

Merged as a4766c9, thanks @unorthodoxgeek.