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

feat: aws integration UI facing api: services #6803

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

raj-k-singh
Copy link
Collaborator

@raj-k-singh raj-k-singh commented Jan 12, 2025

Summary

Adds APIs for listing and configuring cloud services for AWS integration

Related Issues / PR's

Contributes to #6544


Important

Add APIs and models for AWS cloud service integration, including service listing, detail retrieval, and configuration updates, with corresponding tests.

  • APIs:
    • Add ListServices, GetServiceDetails, and UpdateServiceConfig functions in controller.go for AWS cloud services.
    • Add HTTP handlers in http_handler.go for listing, retrieving details, and updating service configurations.
  • Models:
    • Add CloudServiceSummary, CloudServiceDetails, and CloudServiceConfig in model.go.
  • Repositories:
    • Add serviceConfigRepository interface and implementation in serviceConfigRepo.go.
  • Service Definitions:
    • Add service definitions for EC2 and RDS Postgres in serviceDefinitions/aws/.
  • Tests:
    • Add integration tests in signoz_cloud_integrations_test.go for service listing, detail retrieval, and configuration updates.
  • Misc:
    • Rename repo.go to accountsRepo.go and InitSqliteDBIfNeeded to initAccountsSqliteDBIfNeeded.

This description was created by Ellipsis for d6da8a6. It will automatically update as commits are pushed.

@raj-k-singh raj-k-singh force-pushed the feat/aws-integration-ui-facing-api-1 branch from 6cf92b5 to dc5f991 Compare January 12, 2025 10:30
@github-actions github-actions bot added the enhancement New feature or request label Jan 12, 2025
@@ -0,0 +1,30 @@
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Placeholder data for now. To be detailed in follow up changes

@@ -0,0 +1,30 @@
{
"id": "ec2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Placeholder data for now. To be detailed in follow up changes

@raj-k-singh raj-k-singh force-pushed the feat/aws-integration-ui-facing-api-1 branch from dc5f991 to 917cdd6 Compare January 14, 2025 14:29
@raj-k-singh raj-k-singh marked this pull request as ready for review January 14, 2025 14:30
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 917cdd6 in 1 minute and 41 seconds

More details
  • Looked at 1430 lines of code in 14 files
  • Skipped 2 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/tests/integration/signoz_cloud_integrations_test.go:130
  • Draft comment:
    Consider adding tests for error scenarios, such as configuring a service for a non-connected account or an unsupported service, to improve coverage.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test function TestAWSIntegrationServices is missing tests for error scenarios, such as trying to configure a service for a non-connected account or an unsupported service. Adding these tests would improve coverage and robustness.
2. pkg/query-service/app/cloudintegrations/serviceConfigRepo.go:1
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable in other files as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_k9zPriTPQVEAK70X


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

cloudAccountId = &cloudAccountIdQP
}

resp, apiErr := aH.CloudIntegrationsController.GetServiceDetails(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

connection status in service details will be taken up in a follow up change

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on d6da8a6 in 27 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. pkg/query-service/app/cloudintegrations/controller_test.go:183
  • Draft comment:
    Good practice to check for CloudAccountId not being nil before dereferencing.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the test TestConfigureService adds a check for CloudAccountId being not nil before comparing it with testCloudAccountId. This is a good practice to avoid potential nil pointer dereference errors.

Workflow ID: wflow_xBtlFjCbqZlFsVPp


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant