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

chore: Keep ip only within properties #17674

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Conversation

tiina303
Copy link
Contributor

@tiina303 tiina303 commented Sep 28, 2023

Problem

Trying to clean up our types. See also PostHog/plugin-scaffold#56
We don't need ip to be separate and it just creates confusion.
This is safe because the only apps that (should be) using ip are geoIP and advanced geoIP, both of which check both event.ip and event.properties.$ip
https://github.com/PostHog/posthog-plugin-geoip/blob/main/index.ts#L42 & https://github.com/paolodamico/posthog-app-advanced-geoip/blob/91b845841859fe8ec5a6f27c07df4ee3b43b0bde/index.ts#L87 respectively

Changes

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

How did you test this code?

There are already existing tests e.g.

test('ip override', async () => {

@tiina303 tiina303 force-pushed the keep-ip-in-properties-only branch 3 times, most recently from 8b361b2 to 7203d36 Compare September 28, 2023 15:39
@tiina303 tiina303 force-pushed the keep-ip-in-properties-only branch from 7203d36 to 966d5cf Compare September 28, 2023 15:48
@tiina303 tiina303 marked this pull request as ready for review September 28, 2023 18:43
Copy link
Contributor

@bretthoerner bretthoerner left a comment

Choose a reason for hiding this comment

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

Nice, good idea.

@bretthoerner
Copy link
Contributor

Tests failed but it seems unrelated, I kicked the job.

@tiina303
Copy link
Contributor Author

Tests failed but it seems unrelated, I kicked the job.

we run 4 different replicas of the functional tests - unless one touched anything that could break there as long as one of them passes we should be all good :)

@tiina303 tiina303 merged commit bf8dcd7 into master Oct 5, 2023
70 of 71 checks passed
@tiina303 tiina303 deleted the keep-ip-in-properties-only branch October 5, 2023 19:01
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