Skip to content
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

Added the cro reports to es #685

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

athiruma
Copy link
Collaborator

Type of change

Note: Fill x in []

  • bug
  • enhancement
  • documentation
  • dependencies

Description

  1. Added the Azure reports to es.
  2. Added the single cro_run file making it generic by calling the cro_object class, which has methods that return CRO classes.

For security reasons, all pull requests need to be approved first before running any automated CI

@athiruma athiruma added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file ok-to-test PR ok to test labels Oct 10, 2023
@athiruma athiruma requested a review from ebattat October 10, 2023 11:26
@athiruma athiruma self-assigned this Oct 10, 2023
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

I think that we can create same abstract interface class for AWS and Azure

@@ -0,0 +1,66 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have the same abstract class interface for Azure and AWS ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, same interface class used

def __init__(self, region_name: str = ''):
super().__init__()

# Todo All the below methods will be implement in future releases
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing documentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the Todo statement

from cloud_governance.main.environment_variables import environment_variables


class CollectCROReports(AbstractCollectCROReports):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question, why we need separate CRO class for Azure ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, the interface is same

@athiruma athiruma force-pushed the cro_azure_upload_to_es branch from f5499cc to d446cfc Compare October 12, 2023 04:47
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

You have a lot of common folders, need to verify it that correct approach

@athiruma athiruma force-pushed the cro_azure_upload_to_es branch from d446cfc to 5b3ac06 Compare October 13, 2023 15:32
@athiruma
Copy link
Collaborator Author

You have a lot of common folders, need to verify it that correct approach

Yeah sure.
May be I need to re-arrange the files.

@ebattat
Copy link
Collaborator

ebattat commented Oct 15, 2023

@athiruma, are we ok with the nested common folder ?

@athiruma
Copy link
Collaborator Author

@athiruma, are we ok with the nested common folder ?

I am ok with this. Will be renamed it according to the requirements.

Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

/LGTM

@ebattat ebattat merged commit feb9138 into redhat-performance:main Oct 17, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation ok-to-test PR ok to test
Projects
Development

Successfully merging this pull request may close these issues.

2 participants