google / cloud-forensics-utils

Python library to carry out DFIR analysis on the Cloud
Apache License 2.0
454 stars 88 forks source link

Add deleting instance functionality #266

Closed rgayon closed 3 years ago

rgayon commented 3 years ago

Fixes #265 by adding a Delete() method to the gce Disk & Instance objects.

codecov-io commented 3 years ago

Codecov Report

Merging #266 into master will increase coverage by 2.53%. The diff coverage is 70.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   65.39%   67.93%   +2.53%     
==========================================
  Files          14       29      +15     
  Lines         968     2030    +1062     
==========================================
+ Hits          633     1379     +746     
- Misses        335      651     +316     
Flag Coverage Δ
#nosetests 67.93% <70.54%> (+2.53%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...cloudforensics/providers/azure/internal/network.py 24.52% <24.52%> (ø)
libcloudforensics/providers/aws/internal/kms.py 40.00% <40.00%> (ø)
libcloudforensics/providers/gcp/forensics.py 54.54% <48.00%> (-10.46%) :arrow_down:
libcloudforensics/providers/aws/forensics.py 56.16% <53.06%> (-13.61%) :arrow_down:
...cs/providers/gcp/internal/compute_base_resource.py 37.33% <54.28%> (+0.82%) :arrow_up:
libcloudforensics/providers/aws/internal/log.py 73.07% <55.55%> (-4.20%) :arrow_down:
...ibcloudforensics/providers/gcp/internal/compute.py 56.68% <55.81%> (-5.67%) :arrow_down:
...bcloudforensics/providers/gcp/internal/function.py 40.54% <62.50%> (+8.28%) :arrow_up:
...loudforensics/providers/azure/internal/resource.py 65.38% <65.38%> (ø)
...ibcloudforensics/providers/gcp/internal/project.py 61.90% <66.66%> (-3.81%) :arrow_down:
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 375fb7c...f79662c. Read the comment docs.

sa3eed3ed commented 3 years ago

@rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?

rgayon commented 3 years ago

@rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?

So I actually wonder what I should be testing for, if I'm mock'ing gce_image_client.delete() & gce_disk_client.delete() as I'm not using their result whatsoever. This is similar to how gce_image_client.delete() & gce_snapshot_client.delete() don't have unit tests either

sa3eed3ed commented 3 years ago

@rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?

So I actually wonder what I should be testing for, if I'm mock'ing gce_image_client.delete() & gce_disk_client.delete() as I'm not using their result whatsoever. This is similar to how gce_image_client.delete() & gce_snapshot_client.delete() don't have unit tests either

yes thats true, I think what can be tested here is the delete_disks flag to make sure that disks_to_delete is what is expected to get (from the mock with disks.source fields ).

rgayon commented 3 years ago

@rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?

So I actually wonder what I should be testing for, if I'm mock'ing gce_image_client.delete() & gce_disk_client.delete() as I'm not using their result whatsoever. This is similar to how gce_image_client.delete() & gce_snapshot_client.delete() don't have unit tests either

yes thats true, I think what can be tested here is the delete_disks flag to make sure that disks_to_delete is what is expected to get (from the mock with disks.source fields ).

Added a test to check that the disks().delete() method is called

rgayon commented 3 years ago

little things

ack

hacktobeer commented 3 years ago

sa3eed3ed did a solid review, lgtm from me.