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(lambda): return correct number of migrated links from lambda functions #1882

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

halfwhole
Copy link
Collaborator

@halfwhole halfwhole commented Jul 12, 2022

Problem

Currently, the lambda functions to migrate links between users always returns the number of rows affected as 1, regardless of the actual number of links transferred.

Closes #1806

Solution

  • migrate_user_links has been updated to return the actual number of rows affected when updating the urls table
  • As for migrate_url_to_user, it might not be necessary to return the number of rows affected, because if not for an error, there should always be exactly 1 link transferred. Is it ok to just remove this then?

Tests

  • Test migrate_user_links on staging
  • Test migrate_url_to_user on staging

I've tested this locally by running the javascript code on node (without using aws lambda, which I'm not sure how to use yet), and locally it seems to work well

EDIT: have tested invoking the lambda functions both locally and on staging with the help of @yong-jie and it looks good

@yong-jie
Copy link
Member

migrate-url-to-user will alwas only have 1 link transferred. Is it ok to just remove this then?

yup sure thing, we can remove it! 👍

Just for future consideration: in general it probably isn't very harmful to be overly-verbose. In an unlikely scenario where something goes wrong with the DB function, and it transfers more than one link at a time, logging the number returned would have helped us detect the problem early.

Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm. tested together with @halfwhole and verified that it works on go-staging. We noted that the DB function does not get deployed automatically with the rest of the CI/CD pipeline, and therefore we will need to manually update the functions prior to release.

@halfwhole halfwhole merged commit 66e0491 into develop Jul 13, 2022
@halfwhole halfwhole deleted the fix/lambda/migrate-url-counts branch July 13, 2022 09:34
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.

Migration functions does not return accurate count of affected rows
2 participants