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: rewrite validate_proxy_domains celery task as temporal workflow #22363

Merged
merged 22 commits into from
May 21, 2024

Conversation

frankh
Copy link
Contributor

@frankh frankh commented May 20, 2024

Problem

Towards https://github.com/PostHog/product-internal/issues/602

bonus: now using gRPC instead of http

Changes

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

Does this work well for both Cloud and self-hosted?

How did you test this code?

@frankh frankh requested a review from daibhin May 20, 2024 14:27
frontend uses this to update the UI
most should have been schedule_to_close not start to close
@frankh frankh force-pushed the frank/create-temporal-work-for-proxy-service branch from 90134af to cf5940f Compare May 20, 2024 16:11
ruff is refusing to ignore these files, can't reproduce locally 🤷
@frankh frankh marked this pull request as ready for review May 20, 2024 16:36
@frankh frankh requested a review from daibhin May 20, 2024 17:15
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Not a temporal expert and have no real way of testing / running it but everything looks reasonable

Copy link
Member

@fuziontech fuziontech left a comment

Choose a reason for hiding this comment

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

This is some of the most easily readable code I've read here.
Also perfect application of Temporal and grpc 🙏

posthog/temporal/proxy_service/delete.py Outdated Show resolved Hide resolved
@frankh frankh merged commit 06fe736 into master May 21, 2024
76 checks passed
@frankh frankh deleted the frank/create-temporal-work-for-proxy-service branch May 21, 2024 15:14
Copy link

sentry-io bot commented May 21, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ApplicationError: target CNAME doesn't match posthog.temporal.proxy_service.create in wait_f... View Issue
  • ‼️ NXDOMAIN: None of DNS query names exist: test2.frankhamand.com.posthog.svc.cluster.local., test2.frankhaman... posthog.temporal.proxy_service.create in wait_f... View Issue
  • ‼️ AttributeError: certificateStatus posthog.temporal.proxy_service.create in wait_f... View Issue
  • ‼️ ActivityError: Activity task failed posthog.temporal.proxy_service.delete in run View Issue
  • ‼️ NoAnswer: The DNS response does not contain an answer to the question: ph.givelegacy.com. IN CNAME posthog.temporal.proxy_service.create in wait_f... View Issue

Did you find this useful? React with a 👍 or 👎

timgl pushed a commit that referenced this pull request May 24, 2024
…#22363)

* rewrite validate_proxy_domains celery task as temporal workflow

* Fix styling

* Create workflow

* Formatting

* Fix typing for workflow

* Ignore proto import typechecking

* Add type files

* comment

* Add proxy gRPC address

* return serialized response from create

frontend uses this to update the UI

* Fix timeouts

most should have been schedule_to_close not start to close

* Error state

* fix formatting

ruff is refusing to ignore these files, can't reproduce locally 🤷

* Fix erroring type

* remove old proxy celery task

* Add delete workflow

* Add missing import

* Fix proxy workflows not being registered to worker

* Add missing workflow method

* fix serialization error and wait for dns

* fix delete sync_to_async typing issue

* Switch from batch exports queue to new "general-purpose" queue
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

3 participants