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

Code review #1

Open
danielsinai opened this issue Apr 2, 2023 · 0 comments
Open

Code review #1

danielsinai opened this issue Apr 2, 2023 · 0 comments

Comments

@danielsinai
Copy link

danielsinai commented Apr 2, 2023

Code Review: GCP Exporter

First of all great job 🥳
Here are my comments

Architecture

  • Issue: The Bucket design is missing from the architecture.
    Suggestion: Include the Bucket in the design.

  • Issue: The .idea folder should be removed.
    Suggestion: Add the .idea folder to the .gitignore file.

  • Issue: Redundant code should be removed and not commented.
    Suggestion: Create a separate branch for cleanup instead of polluting the main branch.

Code

  • Issue: Some log/variable names are still related to AWS instead of GCP.
    Suggestion: Update log/variable names to be relevant to GCP.

  • Issue: The boto3 AWS SDK is unnecessary.
    Suggestion: Remove the boto3 library.

  • Issue: The names resource_handler.py and resources_handler.py are not intuitive.
    Suggestion: Consider renaming these files for better readability.

  • Issue: The skip_delete flag appears redundant in resources_handler.py.
    Suggestion: Assess the need for the skip_delete flag and remove if unnecessary.

  • Issue: The resources_handler.py file contains a "God Class" with mixed responsibilities.
    Suggestion: Separate the logic for entities and GCP resources, as well as pagination.

  • Issue: Duplicate logic exists for building the secret name in config.py.
    Suggestion: Extract the duplicate logic to a function.

  • Issue: There is no retry mechanism for failed pagination attempts.
    Suggestion: Implement a retry mechanism to ensure data is available on subsequent schedules.

  • Issue: The Google Assets API has a high quota, but there are no additional threads.
    Suggestion: Add more threads and provide guidance for users on how to modify the GCP function.

  • Issue: MAX_DELETE_THREADS is redundant in consts.py.
    Suggestion: Remove the unnecessary constant.

Deployment & Documentation

  • Issue: There is no ability to deploy the service account with the terraform
    Suggestion: Provide a method to deploy the service account using the terraform.

  • Issue: VM permissions or admin read-only access for the assets API is unclear.
    Suggestion: Clarify the required access level and, if possible, provide a way to limit access.

  • Issue: The use of JQ configuration to parse GCP assets API data is unclear.
    Suggestion: Add clear instructions for using JQ configuration to parse the data.

  • Issue: The provided Terraform file does not support creating the required bucket.
    Suggestion: Update the Terraform file to include bucket creation functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant