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

Count on Issue labeled, removed unecessary get_installation_id function, and disabled Sentry in local env #39

Merged
merged 9 commits into from
Mar 9, 2024

Conversation

nikitamalinov
Copy link
Contributor

@nikitamalinov nikitamalinov commented Mar 9, 2024

  • Activating Sentry when not on local/dev (Set your ENV in vscode to "local")
  • Remove unused get_installation_id
  • Added increment_request_count to count number of requests from a given owner

@nikitamalinov nikitamalinov changed the title Nikita requests Request Count on Issue labeled, removed unecessary get_installation_id function, and disabled Sentry in local env Mar 9, 2024
@nikitamalinov nikitamalinov changed the title Request Count on Issue labeled, removed unecessary get_installation_id function, and disabled Sentry in local env Count on Issue labeled, removed unecessary get_installation_id function, and disabled Sentry in local env Mar 9, 2024
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
services/webhook_handler.py Outdated Show resolved Hide resolved
def increment_request_count(self, installation_id: int) -> None:
data, _ = self.client.table(table_name="repo_info").select("*").eq(column="installation_id", value=installation_id).execute()
if (data[1] and data[1][0]):
self.client.table(table_name="repo_info").update(json={"requests": data[1][0]['requests'] + 1}).eq(column="installation_id", value=installation_id).execute()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If for some reason it fails, I would like the log to be worn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this should be the case with the other two functions in the supabase_manager right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but the other functions can be modified next time.

Also I was a little vague in my expression. While the error log should be sent out, this process itself is not related to the main process, so it is better not to raise an error. If it is placed at the end of the main process, it is OK to raise the error.

Copy link
Collaborator

@hiroshinishio hiroshinishio Mar 9, 2024

Choose a reason for hiding this comment

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

  1. If it involves database changes, I would like to be able to see it in the PR.
  2. Also, we need to ensure that the changes are included in the production DB along with the PR, or it will cause production errors. Is there a good way to ensure this?
  3. Also, this would out-date everyone's dev and stg environments in Supabase except who raised this PR. We can handle that right now, but is there a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no DB change, that's why I am doing requests only and i/o of tokens later. I think for your #3 it makes sense to migrate right before deploying. If it's a breaking DB change, we can set aside a time when no one is using our product to implement those changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Really? I don't think requests and invoice_no columns existed before...

@hiroshinishio hiroshinishio merged commit 4f4d586 into main Mar 9, 2024
1 check passed
@hiroshinishio hiroshinishio deleted the nikita-requests branch March 9, 2024 18:46
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.

2 participants