-
Notifications
You must be signed in to change notification settings - Fork 14
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
Spot Usage analysis on aws account #637
Conversation
4269bea
to
7b9a9f5
Compare
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.
@athiruma, what about unittest/integration test against Athena ?
from cloud_governance.main.environment_variables import environment_variables | ||
|
||
|
||
class AthenaOperations: |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the query in sensitive data and we should put it under environment variable
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to import the abstract class ?
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.
NO
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about test for boto3 client athena class ?
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.
No need, one method is enough.
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.
I think you should create mock per class and not aggregate both to 1 mock
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.
I created two patches, that wrapped by mock_athena
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.
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): |
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.
Here it is
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]