Spot Usage analysis on aws account #637
Conversation
athiruma
added
documentation
athiruma
force-pushed
the
spot_athena
branch
2 times, most recently
from
4269bea
to
7b9a9f5
Compare
| from cloud_governance.main.environment_variables import environment_variables | ||
|
|
||
|
|
||
| class AthenaOperations: |
There was a problem hiding this comment.
I think we should split it to 2 classes or inheritance from the same interface:
AthenaOperations
PyAthenaOperations
When you have more queries like delete, update, insert. it will cause to confusing.
There was a problem hiding this comment.
@ebattat Added the AbstarctAthenaOperations for the PyAthenaOperations, Boto3ClientAthenaOperations
| def __init__(self, region_name: str = 'us-east-1'): | ||
| self.__s3_operations = S3Operations(region_name=region_name) | ||
|
|
||
| def get_dataframe_from_csv_file(self, file_path: str): |
There was a problem hiding this comment.
Please add typecheck to all the methods in PandasOperations class
| year = current_date.year | ||
| current_month = current_date.month | ||
| previous_month = current_month - 1 if current_month - 1 != 0 else 12 | ||
| query = f""" |
There was a problem hiding this comment.
I think the query in sensitive data and we should put it under environment variable
There was a problem hiding this comment.
it can be open to all. Only database and table names should be private.
No, the need for an integration test here. Maybe I can add unittest. |
| @@ -0,0 +1,15 @@ | |||
| from cloud_governance.common.clouds.aws.athena.abstract_athena_operations import AbstractAthenaOperations | |||
There was a problem hiding this comment.
do we need to import the abstract class ?
| @@ -0,0 +1,15 @@ | |||
| from cloud_governance.common.clouds.aws.athena.abstract_athena_operations import AbstractAthenaOperations | |||
| from cloud_governance.common.clouds.aws.athena.pyathena_operations import PyAthenaOperations | |||
There was a problem hiding this comment.
What about test for boto3 client athena class ?
There was a problem hiding this comment.
No need, one method is enough.
There was a problem hiding this comment.
I think you should create mock per class and not aggregate both to 1 mock
There was a problem hiding this comment.
I created two patches, that wrapped by mock_athena
There was a problem hiding this comment.
its not clear because the mock class with different name from original one
| with patch.object(PyAthenaOperations, 'execute_query', mock_execute_query), \ | ||
| patch.object(BotoClientAthenaOperations, 'execute_query', mock_execute_query): |
Spot usage helps us to identify the spot utilization & savings across accounts linked to the top account level.
Adding the PandasOperatins which is not used, but helps in future releases.
Upgrade boto3=1.26.1 -> 1.26.4 version that would be compatible to the PyAthena[pandas]