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

Migrate to aws sdk 2 for cloudwatch/aws cloud provider #730

Merged
merged 15 commits into from
Nov 11, 2024

Conversation

akavatl
Copy link
Contributor

@akavatl akavatl commented Nov 6, 2024

No description provided.

@akavatl akavatl changed the title Migrate to aws sdk 2 for cloudwatch provider Migrate to aws sdk 2 for cloudwatch/aws cloud provider provider Nov 6, 2024
@akavatl akavatl changed the title Migrate to aws sdk 2 for cloudwatch/aws cloud provider provider Migrate to aws sdk 2 for cloudwatch/aws cloud provider Nov 6, 2024
pkg/backends/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
pkg/backends/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
pkg/backends/cloudwatch/cloudwatch_test.go Show resolved Hide resolved
pkg/cloudproviders/aws/aws.go Outdated Show resolved Hide resolved

p.logger.WithField("ips", IP).Debug("Looking up instances")
err := p.Ec2.DescribeInstancesPagesWithContext(ctx, input, func(page *ec2.DescribeInstancesOutput, lastPage bool) bool {
hasPagesRemaining := true
for hasPagesRemaining {
pages++
Copy link
Member

Choose a reason for hiding this comment

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

I know this is unchanged but just wondering if it should count a page if the describe failed later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair question, don't have a strong opinion tbh

@irisgve
Copy link
Member

irisgve commented Nov 6, 2024

@akavatl should we move to ready for review now?

@akavatl
Copy link
Contributor Author

akavatl commented Nov 6, 2024

@irisgve Yes I will, I want to do some deeper testing but it shouldn't block this being reviewed(just being merged)

@akavatl akavatl marked this pull request as ready for review November 6, 2024 23:44
irisgve
irisgve previously approved these changes Nov 7, 2024
irisgve
irisgve previously approved these changes Nov 8, 2024
Makefile Outdated
@@ -66,14 +66,14 @@ build-gostatsd-fips:
build-lambda-fips:
@$(MAKE) build PKG=$(LAMBDA_PKG) BINARY_NAME="lambda-extension" GOBUILD_OPTIONAL_FLAGS="-tags fips"

build-all: pb/gostatsd.pb.go tools
Copy link
Member

Choose a reason for hiding this comment

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

This should be fine to leave here since build-all covers building the proto

irisgve
irisgve previously approved these changes Nov 8, 2024
@akavatl akavatl merged commit a5399ae into master Nov 11, 2024
5 checks passed
@akavatl akavatl deleted the migrate-to-aws-sdk-2 branch November 11, 2024 22:03
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.

3 participants