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

Postgres Support #86

Merged
merged 6 commits into from
Dec 9, 2024
Merged

Postgres Support #86

merged 6 commits into from
Dec 9, 2024

Conversation

acx1729
Copy link
Contributor

@acx1729 acx1729 commented Nov 21, 2024

This PR adds support for postgres as as a built-in alternative to sqlite for storing orchestration state. There is an other PR, which appears to have gone stale (#33).

@acx1729
Copy link
Contributor Author

acx1729 commented Nov 25, 2024

@microsoft-github-policy-service agree company="Open Governance Inc."

@cgillum
Copy link
Member

cgillum commented Nov 25, 2024

This is great to see! Is it possible to add some tests to confirm that the Postgres backend is working as designed?

@acx1729
Copy link
Contributor Author

acx1729 commented Nov 26, 2024

Sure! Working on it.

@Mahanmmi
Copy link
Contributor

I've added the tests with using the current test suit for sqlite, the line is commented out to avoid any errors in automation pipelines since it does need a running postgres and I was not sure if or how you are planning to add it to the pipeline, but I tested it locally and it passes all tests.

* feat: add postgres to actions

* fix: remove toolchain

* fix: remove toolchain

* fix: update comment
@acx1729
Copy link
Contributor Author

acx1729 commented Dec 6, 2024

Hi @cgillum

Greetings. Let me know if there is anything else you wish to see in this PR.

Thank you!

@cgillum
Copy link
Member

cgillum commented Dec 7, 2024

@acx1729 @Mahanmmi looks great, thanks! I've kicked off a test run to see if everything works as expected, and everything looks green.

One more thing: I worry a little about the dev experience of running tests locally and requiring postgres to be running. Would it be possible to have the postgres variant run only when a particular environment variable is specified, and then set that variable as part of the CI process? That way testing with postgres can be opt-in. Ideally, we'd document this environment variable flag in a README.md file in the backend/postgres directory.

* fix: add conditional postgres test

* fix: multi instance bug
@acx1729
Copy link
Contributor Author

acx1729 commented Dec 8, 2024

Thank you @Mahanmmi
Hopefully we are all OK @cgillum

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks!

@cgillum cgillum merged commit f11d43f into microsoft:main Dec 9, 2024
4 checks passed
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.

3 participants