-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Run posthog-production CI in a way testing migration continuity #1863
Conversation
Q: Could it be worth it to make this into a separate pipeline/check? Rolling it into Backend CI tests would make that check slower and harder to figure out what broke - i.e. was it the tests or the migrations? |
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.
My feedback:
- From L236 shouldn't we also run tests for EE? We fully use EE features on PostHog Cloud.
- Perhaps take the opportunity to change all references (e.g. L141/L142) from multi tenancy to cloud?
- From the code it was a bit unclear the purpose of checking out master first and then current, maybe add some comment lines?
Addressing @paolodamico:
|
Great! So just re 1, what I meant is that on these |
@paolodamico Not sure if this now runs EE tests the way you meant? Do say if there's an enhancement to be made still. |
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.
small change to make sure EE tests are run in posthog-production
context too but looks pretty good!
Changes
This changes the posthog-production GitHub Actions test on this repo to check if migrating from current master to the PR is possible. I caught this issue of migrations working just fine from scratch but not from master a couple of times before merging in the past, but this should 100% ensure that such a bug will never slip through.