-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add a way to run tests in PostgreSQL in Docker #3699
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of suggestions to help with maintaining this stuff...
CONTRIBUTING.rst
Outdated
build, this will be shown in GitHub, so please keep an eye on the pull request | ||
for feedback. | ||
|
||
To run unit tests, you can use: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest "To run unit tests on a development environment ..." to distinguish from the previous paragraph about CI and PRs
Dockerfile-pgtests
Outdated
@@ -0,0 +1,8 @@ | |||
FROM matrixdotorg/sytest:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we stick the Dockerfile in the docker
directory to avoid cluttering the top-level more than necessary?
MANIFEST.in
Outdated
@@ -28,6 +28,9 @@ exclude jenkins*.sh | |||
exclude jenkins* | |||
exclude Dockerfile | |||
exclude .dockerignore | |||
exclude Dockerfile-pgtests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably redundant if the dockerfile is in the docker
dir?
test_postgresql.sh
Outdated
@@ -0,0 +1,3 @@ | |||
#! /usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#!/bin/bash
is conventional for shell scripts.
Does it need a set -e
so that it doesn't plough on with the run
if the build
fails?
docker_run_pg_tests.sh
Outdated
@@ -0,0 +1,10 @@ | |||
export PGDATA=/var/lib/postgresql/data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do with a shebang line and a comment saying what it does?
and a set -e
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it go in a subdir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Dockerfile-pgtests
Outdated
@@ -0,0 +1,8 @@ | |||
FROM matrixdotorg/sytest:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it also have a comment saying what it does?
@hawkowl bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
CONTRIBUTING.rst
Outdated
the Jenkins builds require an adminstrator to start them. If your change | ||
breaks the build, this will be shown in github, so please keep an eye on the | ||
pull request for feedback. | ||
We use `Jenkins <http://matrix.org/jenkins>`_, `CircleCI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we don't use jenkins any more...)
No description provided.