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(environments): Add new /environments endpoint and move /projects to Project #24154

Merged
merged 37 commits into from
Sep 11, 2024

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Aug 2, 2024

Problem

Step 4 of project environments implementation. Give this a read before reviewing.

Changes

Added the /environments endpoint, which is based on the Team model. At the same time, moved /projects to the Project model, with a backward compatibility feature of "passing through" reads and writes of environment-specific settings to the Team with the same ID as the Project (this means that all existing TestTeam tests should continue to pass).

TODO for self before merging: check if project.name == project.passthrough_team.name currently holds for all projects in both regions.

How did you test this code?

TODO: Add /environments tests.

@Twixes Twixes force-pushed the projects-filtering branch from dd8fdee to 039719f Compare August 2, 2024 09:38
@Twixes Twixes force-pushed the environments-endpoint branch from 6c4eaa8 to 336d8cb Compare August 5, 2024 05:33
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@Twixes Twixes reopened this Aug 19, 2024
Base automatically changed from projects-filtering to master August 19, 2024 15:56
@Twixes Twixes force-pushed the environments-endpoint branch from 616975b to 471c1a3 Compare August 19, 2024 15:58
posthog/api/project.py Dismissed Show dismissed Hide dismissed
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot posthog-bot removed the stale label Aug 20, 2024
Copy link
Contributor

github-actions bot commented Aug 21, 2024

Size Change: 0 B

Total Size: 1.1 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.1 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes requested a review from benjackwhite September 2, 2024 09:54
@Twixes Twixes marked this pull request as ready for review September 2, 2024 09:54
Comment on lines +89 to +98
def register_grandfathered_environment_nested_viewset(
prefix: str, viewset: type[viewsets.GenericViewSet], basename: str, parents_query_lookups: list[str]
) -> tuple[NestedRegistryItem, NestedRegistryItem]:
"""
Register the environment-specific viewset under both /environments/:team_id/ (correct endpoint)
and /projects/:team_id/ (legacy, but supported for backward compatibility endpoint).
DO NOT USE ON ANY NEW ENDPOINT YOU'RE ADDING!
"""
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a key bit

@Twixes Twixes requested review from aspicer and removed request for benjackwhite September 9, 2024 19:19
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@@ -50,3 +56,7 @@ def __str__(self):
return str(self.pk)

__repr__ = sane_repr("id", "name")

@cached_property
def passthrough_team(self) -> "Team":
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider having a "primary_team" field instead of using this hack?

Worried that this has the potential to be leaky - once we get one project with multiple teams, this field might point to another project's team.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is actually a guarantee in place that Team.objects.get(id=project.id) always returns the projects' first team – even for projects with multiple environments. That's because at this point all Project and Team objects use the same source of IDs: Team.objects.increment_id_sequence().

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that it works now, I guess I'm assuming that it won't always be true (or it could always be true and we just allow team ids to have big gaps), so it would be better to not build logic in that depends on that?

Not awful to migrate later when we're ready to diverge, so nbd either way, but just a thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that it will always be true, because it makes migrating things easier in the beginning. E.g. in ingestion, where events will continue to be Team-specific, but group definitions will become Project-specific

Copy link
Contributor

@aspicer aspicer left a comment

Choose a reason for hiding this comment

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

I am not terribly well versed in django internals (router or auth), but from my understanding of the project, this looks good for this step of the project.

Added one comment about potentially making the passthrough relation explicit instead of magic.

@Twixes
Copy link
Member Author

Twixes commented Sep 10, 2024

Thanks for giving this a look @aspicer!

@Twixes Twixes force-pushed the environments-endpoint branch from 8fe7a07 to cf886a6 Compare September 10, 2024 16:41
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes merged commit f2522de into master Sep 11, 2024
91 checks passed
@Twixes Twixes deleted the environments-endpoint branch September 11, 2024 12:05
@Twixes Twixes changed the title feat(environments): Add new environments endpoint and move /projects to Project feat(environments): Add new /environments endpoint and move /projects to Project Sep 11, 2024
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.

3 participants