romanchyla
commented
2 years ago exactly that's my point -- you are making it unnecessarily complicated and complex by insisting on having two endpoints.
having single endpoint is as simple as:
@advertise(scopes=['ads:resolver-service'], rate_limit=[1000, 3600 * 24])
@bp.route('/delete', methods=['DELETE'])
if payload instanceof newProtobuf:
delete_new(...)
else:
delete(...)but since you insist on having two endpoints, then go for it if you believe it superior -- but under these conditions: these adhoc/tmp endpoins will not be deployed to production API (that is big NO NO); and they will not necessitate ad hoc changes to master pipeline (ditto)
to 'squash commits' you can google it
On Tue, Nov 23, 2021 at 10:00 AM golnazads @.***> wrote:
I dont understand the second part "commits squashed". As I have said many times, I shall rename the endpoints, but after master pipeline has used it to populate the new table. I am going to go ahead and ask Nemaja to run the alembic script to create the new table in the database without touching the old table, and then shall wait for Sergi to implement all that needs to be done in the master pipeline. After that, I'm going to deploy the resolver service to dev only and let the master pipeline populate the new table while still populating the old table so that there would be no interruption in the service. Finally I shall examine the queries with having all the data in the new table to make sure they are executed timely.
On Tue, Nov 23, 2021 at 9:35 AM Roman Chyla @.***> wrote:
@.**** approved this pull request.
Approved with stipulation that the new endpoints be renamed and commits squashed
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/adsabs/resolver_service/pull/86#pullrequestreview-813744201 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AG3M4CB7VYLW4W3NQG4LXBTUNORDTANCNFSM5DJTV7QA
. Triage notifications on the go with GitHub Mobile for iOS < https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/adsabs/resolver_service/pull/86#issuecomment-976673162, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADEREWUPZEQ3P6BYLF2OPLUNOT7NANCNFSM5DJTV7QA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
golnazads
I dont understand the second part "commits squashed". As I have said many times, I shall rename the endpoints, but after master pipeline has used it to populate the new table. I am going to go ahead and ask Nemaja to run the alembic script to create the new table in the database without touching the old table, and then shall wait for Sergi to implement all that needs to be done in the master pipeline. After that, I'm going to deploy the resolver service to dev only and let the master pipeline populate the new table while still populating the old table so that there would be no interruption in the service. Finally I shall examine the queries with having all the data in the new table to make sure they are executed timely.
On Tue, Nov 23, 2021 at 9:35 AM Roman Chyla @.***> wrote: