-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add initial migration console snapshot support #751
Add initial migration console snapshot support #751
Conversation
dc24252
to
d8d3641
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #751 +/- ##
============================================
- Coverage 64.90% 64.79% -0.11%
- Complexity 1586 1588 +2
============================================
Files 239 241 +2
Lines 9919 10039 +120
Branches 771 772 +1
============================================
+ Hits 6438 6505 +67
- Misses 3072 3126 +54
+ Partials 409 408 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, after linting and tests are fixed up and this is merged, remaining pieces are:
- implement status
- add to CDK/ECS (I think I forgot to put that one in the task)
and I think that's it?
@@ -61,3 +64,11 @@ def __init__(self, config_file: str): | |||
logger.info(f"Backfill migration initialized: {self.backfill}") | |||
else: | |||
logger.info("No backfill provided") | |||
|
|||
if 'snapshot' in self.config: | |||
self.snapshot: Snapshot = S3Snapshot(self.config["snapshot"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Down the line, we likely want to clean this up and use a factory to give us a s3 snapshot vs local vs whatever, but I think it's good for now.
try: | ||
subprocess.run(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, check=True) | ||
logger.info(f"Snapshot {self.config['snapshot_name']} created successfully") | ||
return CommandResult(success=True, value=f"Snapshot {self.config['snapshot_name']} created successfully") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I kind of forgot that we don't have to do any of the s3 stuff ourselves! 👍
@@ -66,6 +67,35 @@ def test_cli_with_backfill_describe(runner, env, mocker): | |||
assert result.exit_code == 0 | |||
|
|||
|
|||
def test_cli_snapshot_create(runner, env, mocker): | |||
mock_create = mocker.patch('console_link.models.snapshot.Snapshot.create') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work? I think you might have to do a patch object, because create
isn't a static method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by moving to mock the logic function
49946c9
to
959dee9
Compare
Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
959dee9
to
6a47641
Compare
Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
I'll add status in a separate PR as it's tied with making create async |
@@ -66,6 +67,40 @@ def test_cli_with_backfill_describe(runner, env, mocker): | |||
assert result.exit_code == 0 | |||
|
|||
|
|||
def test_cli_snapshot_create(runner, env, mocker): | |||
mock = mocker.patch('console_link.logic.snapshot.create') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the disadvantage of mocking this point instead is that we don't have test coverage for anything in the model, but I'm okay with improving this later.
Description
Adds
console snapshot create
command full stack.Adds stubbing for
console snapshot status
Issues Resolved
Part 1 for MIGRATIONS-1792
Is this a backport? If so, please add backport PR # and/or commits #
Testing
Manual testing in both cloud and local environments.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.