Skip to content
This repository has been archived by the owner on Aug 25, 2023. It is now read-only.

Refactor/adapt legacy single table restore to use Async job #6

Merged
merged 12 commits into from
Jun 28, 2018

Conversation

przemyslaw-jasinski
Copy link
Contributor

No description provided.


app = webapp2.WSGIApplication([
webapp2.Route(
'/restore/table/<project_id:.*>/<dataset_id:.*>/<table_id:.*>',
Copy link
Contributor

Choose a reason for hiding this comment

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

The URL was supposed to be similar to the one from dataset restore so it should be:
/restore/project/<project_id:.>/dataset/<dataset_id:.>/table/<table_id:.*>

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

TableRestoreHandler
),
webapp2.Route(
'/restore/schedule/table/<project_id:.*>/<dataset_id:.*>/<table_id:.*>/<partition_id:.*>',
Copy link
Contributor

Choose a reason for hiding this comment

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

The URL was supposed to be similar to the one from dataset restore so it should be:
/schedule/restore/project/<project_id:.>/dataset/<dataset_id:.>/table/<table_id:.*>

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably don't need partition_id part here ?? it is provided as a request parameter (line 17)

@patch.object(BackupListRestoreService, 'restore')
@patch.object(BackupFinder, 'for_table')
@patch.object(uuid, 'uuid4', return_value=123)
def test_happy_path(self, _, for_table, restore):
Copy link
Contributor

@radkomateusz radkomateusz Jun 28, 2018

Choose a reason for hiding this comment

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

I don't like "test_happy_path" name, I want sth which describe what should happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I did chang the name to 'test_service_call_backup_list_restore_with_one_item_only'.
And removed redundant checks, because it should test only that redirects to backup list restore service.

@coveralls
Copy link

coveralls commented Jun 28, 2018

Pull Request Test Coverage Report for Build 179

  • 67 of 68 (98.53%) changed or added relevant lines in 3 files are covered.
  • 16 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 88.395%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/restore/table/table_restore_handler.py 33 34 97.06%
Files with Coverage Reduction New Missed Lines %
src/big_query/copy_job_service.py 16 0.0%
Totals Coverage Status
Change from base Build 165: 0.7%
Covered Lines: 1988
Relevant Lines: 2249

💛 - Coveralls

@@ -0,0 +1,34 @@
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

What about moving this file to package: src.backup.datastore ?

@przemyslaw-jasinski przemyslaw-jasinski merged commit 270b4d8 into master Jun 28, 2018
@przemyslaw-jasinski przemyslaw-jasinski deleted the YACHT_753 branch June 28, 2018 12:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants