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

fix: tracking job offer update events #194

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

AquiGorka
Copy link
Contributor

Review Type Requested (choose one):

  • Glance - superficial check (from domain experts)
  • Logic - thorough check (from everybody doing review)

Summary

These changes make it so that tracking the job offer update is not a subscription to job offer updates but a method call right before the subscriptions are executed.

Details (optional)

Why?
I wish I knew why the previous way was not working, locally it worked, so the assumption was that it would also work in the deployed infra.
I have a hypothesis the previous subscription was part of the client code and because the client should not have access to the API_HOST env var the requests should not have been going out, but as it happens, the doppler settings for run did include the API_HOST - although, after removing it, locally the events would be still tracked 🤷 .

How to test this code? (optional)

  • Setup a local network
  • Spin up the api server
  • Trigger both a cli jobn and an onchain job, then run the integration tests
  • With each execution verify the jobs table in the db has the corresponding job entry (in the api repo:./stack psql, then SELECT * FROM jobs;)

@@ -16,8 +16,9 @@ const nodeInfoEndpoint = "nodes"
const nodeConnectionEndpoint = "uptimes"
const dealsEndpoint = "deals"

var host = os.Getenv("API_HOST")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@walkah I moved this out of the function call, I don't think we need to call it every time the function executes right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're absolutely right! Good catch 👍

Copy link
Collaborator

@walkah walkah left a comment

Choose a reason for hiding this comment

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

🚀 lgtm!

@@ -16,8 +16,9 @@ const nodeInfoEndpoint = "nodes"
const nodeConnectionEndpoint = "uptimes"
const dealsEndpoint = "deals"

var host = os.Getenv("API_HOST")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're absolutely right! Good catch 👍

@AquiGorka AquiGorka merged commit 0c2163d into main Jun 26, 2024
1 check passed
@AquiGorka AquiGorka deleted the gorka/fix-tracking-job-offer-update-events branch June 26, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants