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

feat(person-on-events): Enable CI to run using both old and new queries #9814

Merged
merged 21 commits into from
May 19, 2022

Conversation

neilkakkar
Copy link
Contributor

@neilkakkar neilkakkar commented May 17, 2022

Problem

All the groundwork needed for person on events querying

Step 1 of many, as referenced here: #9802

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@neilkakkar neilkakkar marked this pull request as ready for review May 17, 2022 15:28
@neilkakkar neilkakkar requested a review from EDsCODE May 17, 2022 15:29
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Some materialization error and comment below otherwise makes sense!

i=index
)
)

#  use person properties mapping to populate person properties in given event
if person_mapping and person_mapping.get(event["distinct_id"]):
Copy link
Member

Choose a reason for hiding this comment

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

Going to need to handle group properties in some way like this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, leaving it for a PR where groups get tackled, as that provides an easy way to test the changes are sound.

Right now, since there's no code actually using the instance setting, the new CI stuff still runs the old code, with extra event properties populated.

@EDsCODE
Copy link
Member

EDsCODE commented May 17, 2022

Another thing is snapshots won't be nice. The person on event tests will all expect different snapshots

@neilkakkar
Copy link
Contributor Author

Another thing is snapshots won't be nice. The person on event tests will all expect different snapshots

These tests would now disregard snapshot changes. Good call. Have added direction in the main issue to include atleast one test with new snapshots.

@neilkakkar neilkakkar requested a review from EDsCODE May 18, 2022 15:15
@neilkakkar neilkakkar mentioned this pull request May 18, 2022
19 tasks
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Confused what the snapshot handling is but otherwise looks good

ENABLE_ACTOR_ON_EVENTS_TEAMS=all pytest ee \
--splits ${{ inputs.concurrency }} \
--group ${{ inputs.group }} \
--snapshot-update
Copy link
Member

@EDsCODE EDsCODE May 18, 2022

Choose a reason for hiding this comment

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

What's supposed to be happening here? Not sure how this is preventing snapshots

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, these updates aren't committed so it never has any impact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep exactly

@neilkakkar neilkakkar merged commit 9750b7d into master May 19, 2022
@neilkakkar neilkakkar deleted the person-events-ci branch May 19, 2022 08:50
fuziontech added a commit that referenced this pull request May 19, 2022
* master:
  refactor(ingestion): Make `KAFKA_ENABLED` true by default and set `KAFKA_HOSTS` default (#9844)
  feat(apps): transpile frontend.tsx (#9828)
  feat: show api call status when adding insights to dashboards (#9817)
  feat: track metrics on zapier hook firings (#9866)
  fix(onboarding): instrumentation (#9845)
  feat(whitelabel-shared-dashboard): Hide branding on shared dashboards paid (#9849)
  fix(apps): plugin source quickfix (#9862)
  refactor(plugin-server): Remove `ts-jest`, use `jest.mocked` (#9861)
  refactor(plugin-server): Trigger public jobs with graphile insted of celery queue (#9811)
  chore: upgrade pip tools (#9859)
  feat(apps): plugin source in its own model, part 2 (#9854)
  chore: Update sprint_planning_retro.md (#9791)
  feat(apps): plugin source in its own model, part 1 (#9853)
  feat(matrix): Add option to save `simulate_matrix` like `setup_dev` (#9836)
  fix(cohort): add mapping from event to person (#9841)
  feat(person-on-events): Enable CI to run using both old and new queries (#9814)
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.

2 participants