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

Add Application Signals Service Level Objective Table #2291

Conversation

johnlayton
Copy link

Integration test logs

Logs
Add passing integration test logs here

Example query results

Results
Add example SQL query results here (please include the input queries as well)

Tags: map[string]string{"service": "application_signals", "action": "GetApplicationSignalsServiceLevelObjective"},
},
},
GetMatrixItemFunc: SupportedRegionMatrix(cloudwatchlogsv1.EndpointsID),
Copy link
Author

@johnlayton johnlayton Sep 2, 2024

Choose a reason for hiding this comment

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

Query, how do I identify regions supported by this application_signals service?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @johnlayton,

Thank you for your contribution! 🎉

In this plugin, we are currently using AWS Go SDK v1, specifically version v1.51.19. Unfortunately, in this version, the AWS Application Signals service is not yet available. After reviewing the AWS Go SDK v1 repository, I found that support for this service was added in version v1.53.20.

To use the EndpointID of the Application Signals service, we would need to update the AWS Go SDK v1 to v1.53.20. This would enable us to leverage the EndpointID reference.

Alternatively, if we prefer not to update the AWS SDK v1, we could potentially use a hardcoded value (application-signals) to retrieve the supported regions for the Application Signals service. However, I’m not entirely certain if using a hardcoded value would fully resolve the issue in listing supported regions for the service.

Please let us know what works best for you.

Thanks again!

Copy link
Author

Choose a reason for hiding this comment

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

Great feedback. Just to confirm (I am new to Go) the go.mod and go.sum changes need to be reverted except for adding github.com/aws/aws-sdk-go-v2/service/applicationsignals v1.3.1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @johnlayton,

I noticed that the current package version we are using in the AWS plugin (v1.27.0) already supports the applicationsignals service. Therefore, there’s no need to update AWS SDK V2 to support this table.

However, for the service endpoint reference, we may need to update AWS SDK V1 to version v1.53.20.

To resolve this, you can follow the steps outlined below:

  1. Revert the AWS SDK V2 to v1.27.0 by running the command: go get github.com/aws/[email protected].
  2. Update AWS SDK V1 to v1.53.20 to obtain the endpoint ID reference by running the command: go get github.com/aws/[email protected].
  3. Replace the hardcoded endpoint ID value here.
  4. Make any necessary adjustments to the aws_application_signals_service_level_objective table, depending on the API behavior, if required.

Please let me know if you need any further clarification.

Thanks!

Copy link
Author

@johnlayton johnlayton Sep 17, 2024

Choose a reason for hiding this comment

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

Hi. I updated the dependencies as suggested. I still see changes to configsources and endpoints, and the linting fails. Is this expected or am I still missing something?

Is it related to this comment -> #2291 (review)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @johnlayton, thank you so much for your efforts and contribution! I’ve merged the PR into a branch.

The lint failure was caused by the use of the deprecated function EndpointResolverWithOptionsFunc with AWS SDK V2 version 1.30.5.

I'll look into resolving it.

Copy link
Contributor

@ParthaI ParthaI left a comment

Choose a reason for hiding this comment

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

Hello @johnlayton,

I’ve left a few initial review comments—whenever you have a moment, please take a look.

Additionally, we are in the process of updating the AWS SDK V2 to version v1.30.4. Could you kindly ensure that this update does not introduce any breaking changes to the existing tables?

Thank you!

Add nil check for service in unsupported region

Co-authored-by: ParthaI <[email protected]>
@johnlayton johnlayton marked this pull request as ready for review September 4, 2024 13:17
@ParthaI ParthaI changed the base branch from main to application-signals-service-level-objective September 17, 2024 13:11
@ParthaI ParthaI merged commit 391ecaa into turbot:application-signals-service-level-objective Sep 17, 2024
1 check failed
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

Successfully merging this pull request may close these issues.

2 participants