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

Run posthog-production CI in a way testing migration continuity #1863

Merged
merged 5 commits into from Nov 10, 2020

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Oct 14, 2020

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.

@timgl timgl temporarily deployed to posthog-test-master-to--krxrch October 14, 2020 07:33 Inactive
@macobo
Copy link
Contributor

macobo commented Oct 14, 2020

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?

Copy link
Contributor

@paolodamico paolodamico left a 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?

@Twixes
Copy link
Collaborator Author

Twixes commented Oct 30, 2020

Addressing @paolodamico:

  1. What part of the test workflow do you mean by also running tests for EE?
  2. Yep, can rename to Cloud, though the repo is still called posthog-production and the main app in there is multi_tenancy, hm…
  3. Will add comments.

@paolodamico
Copy link
Contributor

Great! So just re 1, what I meant is that on these multitenancy tests we're only testing posthog, multi_tenancy & messaging apps (i.e. skipping ee) [see line 236]. My feedback was that maybe we should include CH on multitenancy tests and test ee app too (as since https://github.com/PostHog/posthog-production/pull/40, posthog-production tests rely on CH), but #2062 already introduced this since the last review.

@Twixes
Copy link
Collaborator Author

Twixes commented Nov 8, 2020

@paolodamico Not sure if this now runs EE tests the way you meant? Do say if there's an enhancement to be made still.
@macobo I'd be against having a whole separate check for such a specific thing, I think it fits in quite well with Cloud tests.

Copy link
Contributor

@paolodamico paolodamico left a 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!

@paolodamico paolodamico merged commit 23a72d4 into master Nov 10, 2020
@paolodamico paolodamico deleted the test-master-to-pr-migration branch November 10, 2020 07:47
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.

None yet

4 participants