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: nuke protobuf fully #10932

Merged
merged 6 commits into from
Aug 3, 2022
Merged

chore: nuke protobuf fully #10932

merged 6 commits into from
Aug 3, 2022

Conversation

tiina303
Copy link
Contributor

@tiina303 tiina303 commented Jul 22, 2022

Problem

This is live for a while on cloud and since 1.36.0 for self-hosted (63c783f).

Changes

Note that I nuked the code for 0004 async migration (other than the requirements and precheck part (which helps fail the upgrade if someone is trying to upgrade past it)). I believe rather than wasting time adopting the code that will be never ran (because the idea of async migrations was that we also update the code so that new installs wouldn't need to run them). Hence it would be a waste of everyone's time and resources to keep fixing the code and tests.

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

How did you test this code?

Ran PostHog locally seemed to work fine and saw events coming in.

@macobo
Copy link
Contributor

macobo commented Jul 25, 2022

This does not do what it says it does:

  • posthog/idl folder still exists
  • plugin-server/src/config/idl still exists
  • requirements-dev.in still has types-protobuf
  • requirements.in still has protobuf
  • plugin-server package.json still has protobuf dependencies

Schema snapshots are also unupdated?

@tiina303
Copy link
Contributor Author

tiina303 commented Jul 25, 2022

This does not do what it says it does:

Any preference between adding that to this pr vs updating the title to "... part 1" & following up

Schema snapshots are also unupdated?

Shouldn't there be a failing test?

@macobo
Copy link
Contributor

macobo commented Jul 25, 2022

Any preference between adding that to this pr vs updating the title to "... part 1" & following up

Ask for forgiveness, not permission.

Shouldn't there be a failing test?

Up to you to check - in general schema changes like this should probably result in a snapshot changing. You can verify this locally - maybe this one doesn't?

@tiina303 tiina303 changed the title chore: nuke protobuf fully chore: nuke protobuf fully part 1 Jul 25, 2022
@tiina303 tiina303 changed the title chore: nuke protobuf fully part 1 chore: nuke protobuf fully Aug 1, 2022
@tiina303 tiina303 requested a review from macobo August 2, 2022 11:20
@macobo
Copy link
Contributor

macobo commented Aug 3, 2022

Rebase to fix the plugin server test fail

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

2 participants