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.

Sign up for GitHub

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

Jump to bottom

Part 2: Deprecate old tags and upgrade to new tags Backend #8529

Merged
merged 54 commits into from
Feb 18, 2022
Merged

Part 2: Deprecate old tags and upgrade to new tags Backend #8529

merged 54 commits into from
Feb 18, 2022

Conversation

alexkim205
Copy link
Contributor

@alexkim205 alexkim205 commented Feb 10, 2022 •
edited

Part 1: #8528
Part 3: #8530

PR dependency:

Part 1 <- Part 2 <- Part 3

Changes

Deprecates old tag field in models that were taggable and upgrade them to use new taggable model.

This PR gives these models a facelift:

  • Dashboard
  • Insight
  • Event Definition
  • Property Definition

@alexkim205 alexkim205 mentioned this pull request Feb 10, 2022
Merged
6 tasks
@alexkim205 alexkim205 changed the base branch from master to feat/tags February 10, 2022 02:15
@alexkim205 alexkim205 mentioned this pull request Feb 10, 2022
Closed
2 tasks
@alexkim205

This comment was marked as resolved.

Sign in to view
@alexkim205 alexkim205 mentioned this pull request Feb 10, 2022
Closed
3 tasks
@alexkim205 alexkim205 changed the title Part 2: Deprecate old tags and upgrade to new tags Part 2: Deprecate old tags and upgrade to new tags Backend Feb 11, 2022
@alexkim205 alexkim205 marked this pull request as ready for review February 16, 2022 23:31
mariusandra
mariusandra approved these changes Feb 17, 2022
View reviewed changes
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Everything seems to work for me! 👍

posthog/api/insight.py Show resolved Hide resolved
@alexkim205 alexkim205 enabled auto-merge (squash) February 17, 2022 17:24
rcmarron
rcmarron previously requested changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@rcmarron rcmarron left a comment

Choose a reason for hiding this comment

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

I think the migrations need a bit of attention (details in the comments), but looks good otherwise! Most of the comments are just some nits.

posthog/migrations/0212_deprecated_old_tags.py Outdated
field=django.contrib.postgres.fields.ArrayField(
base_field=models.CharField(max_length=32),
blank=True,
db_column="tags",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really see why we're not just keeping the column name in sync with the field name. Making it obvious that the field is deprecated at the DB layer feels like the thing we should do. Otherwise, future us are going to be doing some analytics from metabase and end up counting the wrong tag. And it's just nice to keep django fields in sync with column names.

Also, if I'm missing something and we do want to keep the db column name, RenameField updates the column name, so here we're renaming the column name twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense. For renaming columns I don't think anything could go wrong with renaming the column since the codebase is a monolith and we're changing all references to "tags" anyways

posthog/migrations/0213_migrate_dashboard_insight_tags.py Outdated Show resolved Hide resolved
ee/migrations/0010_migrate_definitions_tags.py Outdated Show resolved Hide resolved
ee/migrations/0010_migrate_definitions_tags.py Outdated Show resolved Hide resolved
ee/migrations/0008_deprecated_old_tags.py Outdated Show resolved Hide resolved
posthog/migrations/0213_migrate_dashboard_insight_tags.py Outdated Show resolved Hide resolved
ee/test/test_migration_0010.py Show resolved Hide resolved
posthog/test/base.py Show resolved Hide resolved
posthog/api/dashboard.py Outdated Show resolved Hide resolved
posthog/api/organization.py Outdated Show resolved Hide resolved
rcmarron
rcmarron approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@rcmarron rcmarron left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexkim205
Copy link
Contributor Author

@mariusandra We made a decision to rename the old tag columns here from tags to deprecated_tags (see discussion inline), but will wait on merging before your review. Do you forsee anything that can go wrong here?

@mariusandra
Copy link
Collaborator

🚢

@mariusandra
Copy link
Collaborator

In case the boat was ambiguous, looks good to me! Some merge conflicts though 🤔

@alexkim205 alexkim205 merged commit d7a0c10 into master Feb 18, 2022
@alexkim205 alexkim205 deleted the feat/tags-in-old-places branch February 18, 2022 16:47
timgl added a commit that referenced this pull request Feb 18, 2022
@timgl
Revert "Part 2: Deprecate old tags and upgrade to new tags Backend (#…
2c5e277 
…8529)"

This reverts commit d7a0c10.
@timgl timgl mentioned this pull request Feb 18, 2022
Closed
EDsCODE added a commit that referenced this pull request Feb 18, 2022
@EDsCODE
Merge branch 'master' into filter-groups-simplify
68c538b 
* master:
  hobby: Wait for ClickHouse and for Postgres before starting (#8686)
  Part 2: Deprecate old tags and upgrade to new tags Backend (#8529)
  Remove flake8-commas (#8695)
  Update Breakdown props to use filter groups (#8679)
  Automatically switch to the right project if possible (#8681)
  Super Lazy VMs (#8609)
  .github/workflows/ci-backend.yml: fix flake8 config (#8676)
  Fix recording page refresh loop (#8685)
  Instance status configuration (#8096)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariusandra mariusandra mariusandra approved these changes

@rcmarron rcmarron rcmarron approved these changes

Assignees

@alexkim205 alexkim205

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@alexkim205 @mariusandra @rcmarron