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

Add support for sqlite3 to dbfixtures executor #613

Merged
merged 3 commits into from
Jan 4, 2023

Conversation

etandel
Copy link
Contributor

@etandel etandel commented Dec 27, 2022

It seems the underlying libraries used by dbfixtures support sqlite3, so I'm adding support for it as well.

I think the tests are passing locally, that's why I'm opening this PR, but TBH I'm not completely sure: make integration-test exits successfully with 0, but I don't see any venom output in my stdout.

Do let me know if anything else needs changing.

@etandel etandel force-pushed the feat/sqlite-dbfixtures branch from f4a6180 to b01e50a Compare December 27, 2022 08:58
@ovh-cds
Copy link
Collaborator

ovh-cds commented Dec 27, 2022

CDS Report build-venom-a#1061.0 ✘

  • Build
    • Build ✔
    • Unit Tests ✔
  • Tests
    • Acceptance Tests ✘

tests/db_fixtures.yml Outdated Show resolved Hide resolved
@fsamin
Copy link
Member

fsamin commented Dec 27, 2022

Hello,
thank you for your contribution, to run the tests please try the following:

unit tests:

  • make test

integration tests:

  • make build OS=linux ARCH=amd64
  • cp dist/venom.linux-amd64 tests/venom
  • cd tests
  • make start-test-stack (wait a few seconds)
  • make build-test-binary-docker
  • make run-test

you can also give a try to gitpod https://www.gitpod.io/ https://github.com/ovh/venom/blob/master/.gitpod.yml

i run the tests in our CI and it is failing failed to ping database: invalid uri authority on test initialize-postgresql-database-with-migrations (see suggestion)

Signed-off-by: Elias Tandel <elias.tandel@90poe.io>
@etandel etandel force-pushed the feat/sqlite-dbfixtures branch 2 times, most recently from 107307c to 4ae6b48 Compare December 27, 2022 15:28
Signed-off-by: Elias Tandel <elias.tandel@90poe.io>
@etandel etandel force-pushed the feat/sqlite-dbfixtures branch from 4ae6b48 to e7de020 Compare December 27, 2022 16:04
@etandel
Copy link
Contributor Author

etandel commented Dec 27, 2022

Hi @fsamin, thanks a lot for the review and the additional instructions on running the integration tests.

I just pushed some fixes and dbfixture tests are passing locally. There are a few unrelated integration tests that are failing (e.g. SSH), probably due to my local setup.

I also took the liberty of adding your testing instructions to the main README under the Hacking section.

@etandel etandel requested a review from fsamin December 27, 2022 16:06
executors/dbfixtures/README.md Outdated Show resolved Hide resolved
Co-authored-by: François Samin <francois.samin+github@gmail.com>
Signed-off-by: Elias Tandel <elias.tandel@90poe.io>
@etandel etandel force-pushed the feat/sqlite-dbfixtures branch from 8cfff4c to 95d2442 Compare December 29, 2022 14:38
@etandel etandel requested a review from fsamin December 29, 2022 14:38
@fsamin
Copy link
Member

fsamin commented Jan 3, 2023

LGTM

@fsamin fsamin requested a review from yesnault January 3, 2023 08:31
@yesnault
Copy link
Member

yesnault commented Jan 4, 2023

LGTM, thank you @etandel

@yesnault yesnault merged commit 80cff7a into ovh:master Jan 4, 2023
@etandel etandel deleted the feat/sqlite-dbfixtures branch January 4, 2023 12:29
@etandel
Copy link
Contributor Author

etandel commented Jan 4, 2023

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants