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
E2E Integration test environment setup #746
Conversation
…hon3.9+node14+playwright
…integ_test_playwright_python
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.
I tested this locally and it works for me.
Without more Docker expertise I'm not able to comment on the overall approach.
Just one small documentation change to suggest:
- In
README.md
, change one instance ofmathesar_db_1
tomathesar_db
.
I think we should try really hard to sync the environment in the docker images with the environment running on staging. I'm okay with trying to move staging over to Ubuntu, though. At this point, it's a pretty common server OS. OTOH, that might be painful to implement at the moment (not sure about that part). What versions of Debian did you check? Edit: As for the approach, I think we'd want to automate building for publication and publishing in order to make updating and changing things easier. Other than that, it makes sense to me. |
We can maintain the docker files used to create the images for mathesar-base in a sub folder, and automate publishing the images. We will need to create an account for mathesar on docker-hub. These are the contents of the docker files used to create pavish73/mathesar-base images. amd64/arm64 image. tag: latest
amd64 image, tag: latest-all (contains playwright)
Using these directly would make building local images very slow. My suggestion is only to publish an intermediary image.
I don't think this would be hard. We only have to replace the base image used in staging with our new base image. Everything else should work out of the box. Although, we would have to test it out and confirm.
Buster and Bullseye |
I've merged with the latest master in which this is handled. |
66aa352
to
226ce11
Compare
226ce11
to
370a2c5
Compare
Integ tests seem to be failing after I merged with master which only had minor updates to the dockerfile. I'll take a look into this. |
370a2c5
to
3382926
Compare
@mathemancer @kgodey I'm running to an issue which requires your help. I'm not sure how the pipeline passed earlier. I've updated our workflow to record videos, which can be seen in the workflow summary (https://github.com/centerofci/mathesar/actions/runs/1382112450). The error thrown is ' Edit: I think it might be because the application reads from the .env file, and only pytest has different value for db, from setup.cfg file. I guess updating the .env file on the integration test environment should suffice. Let me know your thoughts. Edit2: If the application reads from the .env file, mathesar db should have been created (since I have run migrations on the workflow). So, not sure what's happening here. |
The migrations don't create the |
I understand that the install.py script creates the db, which it reads from the .env file. I did run the script, but our service throws an error stating that the db mathesar is not present. It works fine locally but not on GH, I wanted to check if I missed something on the GH action workflow. |
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.
see the line comment.
setup.cfg
Outdated
DJANGO_DATABASE_URL=postgres://mathesar:mathesar@mathesar_db:5432/mathesar_django | ||
MATHESAR_DATABASES=(mathesar_db_test|postgres://mathesar:mathesar@mathesar_db:5432/mathesar_db_test) |
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.
I think the usual way is for the hostname to be the header of the docker-compose.yml
file section (so db
in this case). I wonder if the version of docker you (we) have locally handles that smoothly, but the version on Github doesn't. Maybe try reverting this change.
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.
It was db
but after the workflow failed, I updated it to mathesar_db
to check if that'll make a change. The result was the same. I could try changing it back again.
I missed this from @mathemancer when reading through the conversation previously.
I don't think we should. I made a deliberate decision to not use Docker in our staging setup so that we'd be deploying (and testing) Mathesar in two different environments on a regular basis. I think it makes sense to keep staging on Debian and Docker-free. |
…integ_test_playwright_python
…integ_test_playwright_python
…integ_test_playwright_python
9bd55f4
to
b7657c3
Compare
…terofci/mathesar into integ_test_playwright_python
I have committed the Dockerfile directly for E2E tests. This currently takes ~17 minutes to run on the workflow, and a lot more time locally. The other approaches require more thought. I also tried to simplify the docker setup by using binaries instead of compiling from source (for python and playwright) but that lead to different issues. I didn't want to go down that rabbit hole again and spend any more time on this PR. |
This has a number of changes and I'm dismissing the approval to avoid accidental merge without further review.
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.
@pavish Looks good. Please fix the issue with the issue label workflow and merge.
@@ -43,6 +43,14 @@ jobs: | |||
cd .github/actions/project_update/ | |||
python project_update.py ${{ github.event.issue.node_id }} --status Blocked | |||
|
|||
- name: Update Waiting issues |
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.
@pavish This change seems like a bad merge.
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.
@kgodey This was added in 12ef48d, which was mentioned here as harmless: #746 (comment)
There wasn't another step with the same name. Should I go ahead and remove this?
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.
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.
I think I got confused and forgot that I added it in this PR.
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.
Thanks. Updated in e5bfc70
Fixes #639
Initial approach:
There are a few things to note:
Update on approach:
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin