Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save SQLite DB file to temp location #1161

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

Pchelolo
Copy link
Contributor

@Pchelolo Pchelolo commented Jul 3, 2019

For testing, we run in SQLite mode in docker and so can't create the test file in current directory because of permissions. Instead, let's create the DB file in OS temp location.

cc @wikimedia/services @akosiaris

@eevans
Copy link
Contributor

eevans commented Jul 3, 2019

Insofar as I understand what insecurely: true is doing (COPY [., .]), this seems reasonable (though I'm not sure I actually understand/appreciate the insecurity).

One alternative would be to create the sqlite database as a unique temp file (the JS equivalent of something like mktemp -t sqlite.XXXXXXXXXX), log the resulting filename in case locating it for debugging is required, and leave it to the OS to clean them up. This might result in more robust tests anyway (for example, ensuring each test run gets a fresh database, keeping the working directory free of detritus, etc)

But to be clear, I'm personally OK with the PR as submitted. 👍

@akosiaris
Copy link

Insofar as I understand what insecurely: true is doing (COPY [., .]), this seems reasonable (though I'm not sure I actually understand/appreciate the insecurity).

insecurely means that it skips the step separating the owner of the files from the execution user. That is, instead of switching running the commands under the runuser the commands will be run using the somebody user. Useful while still changing the image. But it could have been named better I think.

LGTM.

Change-Id: I8f27109aefab164d1f538eda3778fc453286454d
@Pchelolo Pchelolo changed the title Run blubber tests on insecure mode Save SQLite DB file to temp location Jul 5, 2019
@Pchelolo
Copy link
Contributor Author

Pchelolo commented Jul 5, 2019

Thanks @eevans and @akosiaris

I have changed the premise of the PR to create DB file in the temp directory.

@eevans
Copy link
Contributor

eevans commented Jul 5, 2019

Nice; I like it! 👍

@Pchelolo Pchelolo merged commit 440491d into wikimedia:master Jul 5, 2019
@Pchelolo Pchelolo deleted the test_insecure branch July 5, 2019 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants