Upgrade Go and Packages #1
Conversation
abicky
force-pushed
the
upgrade-go-and-packages
branch
2 times, most recently
from
ef9a4d8
to
35d57e5
Compare
abicky
force-pushed
the
upgrade-go-and-packages
branch
from
947157b
to
c6c087c
Compare
|
Warning Rate limit exceeded@abicky has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve significant updates across multiple files to transition from AWS SDK for Go v1 to v2, enhance context management, and modernize the build process. Key modifications include upgrading action versions in GitHub workflows, updating the Go version and dependencies in Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (26)
internal/testing/testutil/testutil.go (1)
8-12: Consider adding input validation.The implementation correctly handles the gomock.InOrder call with the new API. The pre-allocation of the slice capacity is a good optimization. However, consider adding validation for the
callsparameter to prevent panic on nil or empty input.func InOrder(calls ...*gomock.Call) *gomock.Call { + if len(calls) == 0 { + panic("InOrder requires at least one call") + } args := make([]any, 0, len(calls)) for _, call := range calls { args = append(args, call) } gomock.InOrder(args...) return calls[len(calls)-1] }Makefile (2)
22-24: Consider adding more static analysis tools.The vet target is a good addition. Consider enhancing it with additional tools like
golangci-lintfor more comprehensive analysis..PHONY: vet vet: $(MOCKS) - go vet ./... + go vet ./... + @which golangci-lint > /dev/null && golangci-lint run ./... || echo "Consider installing golangci-lint"
26-29: Consider adding error checking.The mock generation target is well-implemented, but could benefit from error checking.
$(MOCKS): $(SRCS) - go generate ./... - # mockgen doesn't update timestamps if the generated code doesn't change - touch $(MOCKS) + @echo "Generating mocks..." + @if ! go generate ./...; then \ + echo "Error: Mock generation failed"; \ + exit 1; \ + fi + @# mockgen doesn't update timestamps if the generated code doesn't change + @touch $(MOCKS) + @echo "Mock generation completed successfully"internal/service/interface.go (1)
9-16: Add documentation for better maintainability.The interface design is solid and follows AWS SDK v2 patterns well. Consider adding documentation:
+// ECSAPI defines the interface for AWS ECS operations. +// It abstracts the AWS SDK v2 ECS client for better testability and flexibility. type ECSAPI interface { + // CreateService creates a new Amazon ECS service. CreateService(context.Context, *ecs.CreateServiceInput, ...func(*ecs.Options)) (*ecs.CreateServiceOutput, error) // ... (document other methods similarly) }.github/workflows/main.yml (1)
37-49: Consider adding explicit version tag for goreleaser-action.While the actions upgrade is good, using
latestfor goreleaser-action version could lead to unexpected breaking changes. Consider pinning to a specific version.- uses: goreleaser/goreleaser-action@v6 with: - version: latest + version: v6.1.0 # or another specific versionAlso, the change from
--rm-distto--cleanaligns with goreleaser v6 best practices.internal/service/definition_test.go (1)
21-24: Consider parameterizing the DesiredCount valueThe test cases have been correctly updated for SDK v2, but there's a repeated pattern where
DesiredCount: 1is hardcoded in all test cases. Consider parameterizing this value to make the tests more maintainable and to potentially test different scenarios.Example refactor:
tests := []struct { name string + desiredCount int32 s ecstypes.Service want *service.Definition }{ { name: "default", + desiredCount: 1, s: ecstypes.Service{ ClusterArn: aws.String("arn:aws:ecs:ap-northeast-1:123456789:cluster/default"), - DesiredCount: 1, + DesiredCount: desiredCount, PropagateTags: "NONE", // ... rest of the fields },Also applies to: 35-38, 49-52, 64-67
internal/capacity/interface.go (2)
12-18: Well-structured interface definition for Auto Scaling operations.The interface follows AWS SDK v2 best practices with context support and variadic options for extensibility.
Consider documenting the interface with GoDoc comments to describe its purpose and usage patterns, especially since this is a new interface that other packages will depend on.
28-36: Consider pagination handling for List operations.The
ListContainerInstancesandListTasksoperations might return paginated results.Consider providing helper methods or documentation on handling pagination for these operations. You might want to create wrapper methods that handle pagination automatically using Go channels or iterators, similar to the patterns used in the AWS SDK v2.
cmd/root.go (1)
66-69: Consider handling errors returned by GetStringThe errors returned by
rootCmd.Flags().GetString("region")andGetString("profile")are currently ignored. Handling these errors can improve robustness by ensuring that any unexpected errors during flag retrieval are properly surfaced.You can apply this diff to handle the errors:
func newConfig(ctx context.Context) (aws.Config, error) { - region, _ := rootCmd.Flags().GetString("region") - profile, _ := rootCmd.Flags().GetString("profile") + region, err := rootCmd.Flags().GetString("region") + if err != nil { + return aws.Config{}, fmt.Errorf("failed to get 'region' flag: %w", err) + } + profile, err := rootCmd.Flags().GetString("profile") + if err != nil { + return aws.Config{}, fmt.Errorf("failed to get 'profile' flag: %w", err) + } return config.LoadDefaultConfig(ctx, config.WithRegion(region), config.WithSharedConfigProfile(profile)) }cmd/terminatespotfleetinstances.go (2)
29-29: Fix grammatical error in flag descriptionThe description for the
batch-sizeflag reads: "The number of instances drained at a once". To improve clarity, consider changing it to "The number of instances drained at once".
39-42: Update error message to reflect configuration initializationThe error message states "failed to initialize a session", but since
newConfig(cmd.Context())initializes a configuration, consider updating the error message to "failed to initialize configuration".internal/capacity/poller.go (1)
46-51: UpdatedReceiveMessagecall with context and correct parametersThe call to
ReceiveMessagecorrectly includes the context parameter and updated types. The parameters are appropriately set to align with AWS SDK v2 requirements.Consider making
VisibilityTimeoutconfigurable rather than hardcoding it to10. This allows for greater flexibility and control over message visibility settings.func (p *SQSQueuePoller) PollOnce(ctx context.Context, callback func([]sqstypes.Message) ([]sqstypes.DeleteMessageBatchRequestEntry, error), waitTimeSeconds int32) error { resp, err := p.sqsSvc.ReceiveMessage(ctx, &sqs.ReceiveMessageInput{ MaxNumberOfMessages: sqsconst.MaxReceivableMessages, QueueUrl: aws.String(p.queueURL), - VisibilityTimeout: 10, + VisibilityTimeout: visibilityTimeout, WaitTimeSeconds: waitTimeSeconds, })You might introduce a parameter or constant for
visibilityTimeoutto enhance configurability.internal/capacity/spotfleetrequest.go (2)
Line range hint
148-156: Handle potential index out-of-range panic.In the
reloadfunction, accessingresp.SpotFleetRequestConfigs[0]without checking if the slice is empty could cause a panic if the slice has no elements.Apply this diff to safely handle the response:
- sfr.SpotFleetRequestConfig = resp.SpotFleetRequestConfigs[0] - sfr.SpotFleetRequestConfigData = resp.SpotFleetRequestConfigs[0].SpotFleetRequestConfig + if len(resp.SpotFleetRequestConfigs) == 0 { + return xerrors.Errorf("no Spot Fleet Request Configs found for ID %s", sfr.id) + } + sfr.SpotFleetRequestConfig = resp.SpotFleetRequestConfigs[0] + sfr.SpotFleetRequestConfigData = sfr.SpotFleetRequestConfig.SpotFleetRequestConfig
Line range hint
162-196: Simplify weighted capacity calculations and support float values.The current implementation does not support float values for weighted capacities and assumes they are integers. Consider updating the logic to handle float values, as AWS allows weighted capacities to be floats.
Apply this diff to support float weighted capacities:
-func (sfr *SpotFleetRequest) weightedCapacity() (int32, error) { - weightedCapacity := int32(0) +func (sfr *SpotFleetRequest) weightedCapacity() (float64, error) { + var weightedCapacity float64 = 0 // Spot Fleet Requests with a LaunchTemplate for _, conf := range sfr.SpotFleetRequestConfigData.LaunchTemplateConfigs { for _, o := range conf.Overrides { - if *o.WeightedCapacity != float64(int32(*o.WeightedCapacity)) { - return 0, xerrors.Errorf("currently float weighted capacities are not supported") - } if weightedCapacity == 0 { - weightedCapacity = int32(*o.WeightedCapacity) + weightedCapacity = *o.WeightedCapacity } - if weightedCapacity != int32(*o.WeightedCapacity) { + if weightedCapacity != *o.WeightedCapacity { return 0, xerrors.Errorf("currently mixed weighted capacities are not supported") } } } // Spot Fleet Requests without a LaunchTemplate for _, spec := range sfr.SpotFleetRequestConfigData.LaunchSpecifications { if spec.WeightedCapacity == nil { continue } - if *spec.WeightedCapacity != float64(int32(*spec.WeightedCapacity)) { - return 0, xerrors.Errorf("currently float weighted capacities are not supported") - } if weightedCapacity == 0 { - weightedCapacity = int32(*spec.WeightedCapacity) + weightedCapacity = *spec.WeightedCapacity } - if weightedCapacity != int32(*spec.WeightedCapacity) { + if weightedCapacity != *spec.WeightedCapacity { return 0, xerrors.Errorf("currently mixed weighted capacities are not supported") } } if weightedCapacity == 0 { weightedCapacity = 1 } return weightedCapacity, nil }internal/service/service.go (2)
94-94: Align waiterMaxDelayvalues with AWS recommendationsThe
MaxDelayvalues for waiters are set to15 * time.Secondand6 * time.Second. Ensure these values are appropriate for your use case and consider AWS SDK recommendations.Review and adjust the
MaxDelayvalues if necessary to optimize waiter performance.Also applies to: 148-148
107-119: Handle errors when stopping and deleting servicesEnsure that errors returned from
stopAndWaitUntilStoppedanddeletemethods are appropriately handled, and consider adding retries or additional error context if operations are prone to transient failures.Consider implementing retry logic or enhanced error handling to improve resilience.
internal/capacity/drainer_test.go (4)
118-118: Usectxvariable instead ofcontext.Background()for consistencyIn line 118, you're using
context.Background()directly. Sincectxis already initialized, consider usingctxto maintain consistency and make it easier to modify the context if needed.Apply this diff to use the existing
ctxvariable:-if err := drainer.Drain(context.Background(), instanceIDs); err != nil { +if err := drainer.Drain(ctx, instanceIDs); err != nil {
248-248: Usectxvariable instead ofcontext.Background()for consistencyIn line 248, you're directly using
context.Background(). Consider using the initializedctxvariable for consistency.Apply this diff:
-entries, err := drainer.ProcessInterruptions(context.Background(), messages) +entries, err := drainer.ProcessInterruptions(ctx, messages)
275-275: Usectxvariable when calling AWS SDK methodsEnsure that you're using the initialized
ctxvariable when making AWS SDK calls for consistency and potential future context adjustments.Apply this diff:
-ecsMock.EXPECT().ListContainerInstances(ctx, gomock.Any(), gomock.Any()). +ecsMock.EXPECT().ListContainerInstances(ctx, gomock.Any(), gomock.Any()).
308-308: Usectxvariable instead ofcontext.Background()for consistencyIn line 308, replace
context.Background()with the initializedctxvariable.Apply this diff:
-_, err = drainer.ProcessInterruptions(context.Background(), []sqstypes.Message{}) +_, err = drainer.ProcessInterruptions(ctx, []sqstypes.Message{})internal/capacity/autoscalinggroup.go (5)
Line range hint
285-292: Handle Deprecated Tagging OperationsThe AWS SDK for Go V2 recommends using the
TagResourceandUntagResourceAPIs for tagging instead ofCreateOrUpdateTagsandDeleteTags, which are considered deprecated. Updating to the newer APIs ensures future compatibility and aligns with best practices.Modify the tagging operations to use
TagResource:-_, err := asg.asSvc.CreateOrUpdateTags(ctx, &autoscaling.CreateOrUpdateTagsInput{ - Tags: []autoscalingtypes.Tag{ - asg.createTag("ecsmec:OriginalDesiredCapacity", fmt.Sprint(*asg.OriginalDesiredCapacity)), - asg.createTag("ecsmec:OriginalMaxSize", fmt.Sprint(*asg.OriginalMaxSize)), - asg.createTag("ecsmec:StateSavedAt", stateSavedAt.Format(time.RFC3339)), - }, -}) +_, err := asg.asSvc.TagResource(ctx, &autoscaling.TagResourceInput{ + ResourceARN: asg.AutoScalingGroupARN, + Tags: []autoscalingtypes.Tag{ + { + Key: aws.String("ecsmec:OriginalDesiredCapacity"), + Value: aws.String(fmt.Sprint(*asg.OriginalDesiredCapacity)), + }, + { + Key: aws.String("ecsmec:OriginalMaxSize"), + Value: aws.String(fmt.Sprint(*asg.OriginalMaxSize)), + }, + { + Key: aws.String("ecsmec:StateSavedAt"), + Value: aws.String(stateSavedAt.Format(time.RFC3339)), + }, + }, +})And similarly update the
DeleteTagscall toUntagResource.Please refer to the AWS SDK documentation for the exact usage and required parameters.
236-240: Adjust Waiter Configuration for ConsistencyIn the
InstanceTerminatedWaiter, consider setting bothMinDelayandMaxDelayto ensure consistent polling intervals. Currently, onlyMaxDelayis set, which might lead to defaultMinDelayvalues that are longer than desired.Modify the waiter options:
waiter := ec2.NewInstanceTerminatedWaiter(asg.ec2Svc, func(o *ec2.InstanceTerminatedWaiterOptions) { - o.MaxDelay = 15 * time.Second + o.MinDelay = 5 * time.Second + o.MaxDelay = 10 * time.Second })This adjustment provides more control over the polling behavior during the wait operation.
Line range hint
316-338: Consider Making Wait Timeout ConfigurableThe
waitUntilInstancesInServicefunction uses a hardcoded timeout of 5 minutes. Making this timeout configurable allows for greater flexibility, especially in environments where instance startup times may vary.Modify the function to accept a timeout parameter:
-func (asg *AutoScalingGroup) waitUntilInstancesInService(ctx context.Context, capacity int32) error { +func (asg *AutoScalingGroup) waitUntilInstancesInService(ctx context.Context, capacity int32, timeout time.Duration) error { // WaitUntilGroupInService doesn't work even if the MinSize is equal to the DesiredCapacity // (https://github.com/aws/aws-sdk-go/issues/2478), // so wait manually ticker := time.NewTicker(10 * time.Second) defer ticker.Stop() - timeout := 5 * time.Minute timer := time.NewTimer(timeout) defer timer.Stop()Then, when calling the function, specify the desired timeout:
if err := asg.waitUntilInstancesInService(ctx, *asg.DesiredCapacity); err != nil { // ... }Update the calls to include the timeout value:
+timeout := 5 * time.Minute +if err := asg.waitUntilInstancesInService(ctx, *asg.DesiredCapacity, timeout); err != nil { // ... }This change enhances the flexibility of the function without altering existing behavior.
Line range hint
299-306: Update ResourceType Field to Use ConstantIn the
createTagmethod, theResourceTypefield is set using a string literal"auto-scaling-group". To improve maintainability and reduce the risk of typos, consider using the provided constants from the AWS SDK.Update the
ResourceTypeassignment:func (asg *AutoScalingGroup) createTag(key string, value string) autoscalingtypes.Tag { return autoscalingtypes.Tag{ Key: aws.String(key), PropagateAtLaunch: aws.Bool(false), ResourceId: asg.AutoScalingGroupName, - ResourceType: aws.String("auto-scaling-group"), + ResourceType: autoscalingtypes.ResourceTypeAutoScalingGroup, Value: aws.String(value), } }This change uses
autoscalingtypes.ResourceTypeAutoScalingGroupfor clarity and type safety.
12-16: Organize Imports for Better ReadabilityThe import statements are currently grouped without separation, which can affect readability. Consider organizing imports into standard library packages, third-party packages, and local packages, separated by blank lines.
Reorganize the imports:
import ( "context" "fmt" "log" + "sort" + "strconv" + "time" + "slices" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/autoscaling" autoscalingtypes "github.com/aws/aws-sdk-go-v2/service/autoscaling/types" "github.com/aws/aws-sdk-go-v2/service/ec2" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "golang.org/x/xerrors" "github.com/abicky/ecsmec/internal/const/autoscalingconst" )This grouping enhances readability by clearly separating different categories of imports.
internal/capacity/autoscalinggroup_test.go (1)
Line range hint
83-110: Fix incorrect options type inDofunction forCreateOrUpdateTagsIn the anonymous function passed to
Do, the last parameter is incorrectly specified as...func(*ecs.Options). This should be...func(*autoscaling.Options)to match the expected signature forCreateOrUpdateTags. Using the incorrect type can lead to compilation errors or unexpected behavior.Apply this diff to fix the issue:
asMock.EXPECT().CreateOrUpdateTags(ctx, gomock.Any()).Do(func(_ context.Context, input *autoscaling.CreateOrUpdateTagsInput, _ ...func(*ecs.Options)) { if len(input.Tags) != 3 { t.Errorf("len(input.Tags) = %d; want %d", len(input.Tags), 3) } // ... })should be:
asMock.EXPECT().CreateOrUpdateTags(ctx, gomock.Any()).Do(func(_ context.Context, input *autoscaling.CreateOrUpdateTagsInput, _ ...func(*autoscaling.Options)) { if len(input.Tags) != 3 { t.Errorf("len(input.Tags) = %d; want %d", len(input.Tags), 3) } // ... })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
- .github/workflows/main.yml (2 hunks)
- .gitignore (1 hunks)
- .goreleaser.yml (2 hunks)
- Makefile (2 hunks)
- cmd/recreateservice.go (2 hunks)
- cmd/reduceclustercapacity.go (5 hunks)
- cmd/replaceautoscalinggroupinstances.go (2 hunks)
- cmd/root.go (3 hunks)
- cmd/terminatespotfleetinstances.go (2 hunks)
- go.mod (1 hunks)
- internal/capacity/autoscalinggroup.go (15 hunks)
- internal/capacity/autoscalinggroup_test.go (24 hunks)
- internal/capacity/capacity_test.go (1 hunks)
- internal/capacity/drainer.go (9 hunks)
- internal/capacity/drainer_test.go (7 hunks)
- internal/capacity/interface.go (1 hunks)
- internal/capacity/poller.go (2 hunks)
- internal/capacity/spotfleetrequest.go (8 hunks)
- internal/capacity/spotfleetrequest_test.go (10 hunks)
- internal/service/definition.go (2 hunks)
- internal/service/definition_test.go (1 hunks)
- internal/service/interface.go (1 hunks)
- internal/service/service.go (5 hunks)
- internal/service/service_test.go (8 hunks)
- internal/sliceutil/sliceutil.go (0 hunks)
- internal/sliceutil/sliceutil_test.go (0 hunks)
- internal/testing/capacitymock/generate.go (1 hunks)
- internal/testing/mocks/capacity.go (0 hunks)
- internal/testing/mocks/generate.go (0 hunks)
- internal/testing/servicemock/generate.go (1 hunks)
- internal/testing/testutil/testutil.go (1 hunks)
💤 Files with no reviewable changes (4)
- internal/sliceutil/sliceutil.go
- internal/sliceutil/sliceutil_test.go
- internal/testing/mocks/capacity.go
- internal/testing/mocks/generate.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- internal/testing/capacitymock/generate.go
- internal/testing/servicemock/generate.go
🔇 Additional comments (123)
internal/testing/testutil/testutil.go (1)
4-4: LGTM: Import updated to use the official gomock package.The change from
github.com/golang/mock/gomocktogo.uber.org/mock/gomockis correct as this is now the official maintained version of gomock.Makefile (2)
12-12: LGTM!The clean target correctly removes both the binary and generated mock files.
19-21: LGTM! Verify test coverage.The test target correctly ensures mock files are generated before running tests.
✅ Verification successful
Let me try a different approach to verify the mock files and test coverage using just the available tools.
This script will help us:
- Locate all mock files in the repository
- Find all test files
- Check for mock imports in test files
- Look at test patterns and assertions to evaluate test quality
Test implementation looks comprehensive and well-structured
The verification shows:
- Test files exist across all major components (capacity, service)
- Mock imports are properly used in test files where needed
- Tests follow good practices with:
- Table-driven tests
- Subtests using t.Run
- Mock controllers for dependencies
- Coverage of main scenarios and edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all mock files are being used in tests # and that test coverage is adequate. echo "Checking mock file usage in tests:" for mock in $(find . -type f -name 'mocks.go'); do echo "Checking usage of $mock" rg -l "$(basename $(dirname $mock))" --type go --type-add 'go:*.go' -g '*_test.go' done echo -e "\nChecking test coverage:" for pkg in $(go list ./...); do echo "Coverage for $pkg:" go test -cover $pkg doneLength of output: 262
Script:
#!/bin/bash # Find all mock files and test files echo "Mock files in repository:" fd mocks.go echo -e "\nTest files in repository:" fd "_test.go$" echo -e "\nChecking mock imports in test files:" rg "\".*mock.*\"" --type go -g "*_test.go" -l echo -e "\nChecking test patterns and assertions:" rg "(func Test|t\.Run|assert\.|require\.)" --type go -g "*_test.go" -A 1Length of output: 5970
internal/capacity/capacity_test.go (3)
5-5: LGTM! Import updates align with SDK upgrade objectives.The changes appropriately update the imports to use AWS SDK v2 and the newer math/rand/v2 package. The use of an alias for autoscaling types improves code readability.
Also applies to: 7-7, 11-12
16-16: LGTM! Using idiomatic log output suppression.The change to
io.Discardis the recommended way to suppress log output in tests.
20-24: Verify AWS SDK v2 type usage in related test files.The changes correctly use AWS SDK v2 types, but we should verify consistent usage across related test files.
✅ Verification successful
AWS SDK v2 types are consistently used across test files
The verification shows:
- No AWS SDK v1 imports (
github.com/aws/aws-sdk-go/service/autoscaling) were found in any test files- All instances of
autoscalingtypesusage are from AWS SDK v2, consistently used across test files for:
- Instance types
- AutoScalingGroup types
- TagDescription types
- Mock expectations in test files
The migration to AWS SDK v2 types appears complete and consistent in the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining AWS SDK v1 imports in test files # and verify consistent use of autoscalingtypes # Check for any remaining v1 SDK imports in test files echo "Checking for AWS SDK v1 imports in test files..." rg -t go "github.com/aws/aws-sdk-go/service/autoscaling" --files-with-matches "**/*_test.go" # Check for consistent use of autoscalingtypes alias echo "Checking autoscalingtypes alias usage..." rg -t go "autoscalingtypes\." "**/*_test.go" -C 2Length of output: 448
Script:
#!/bin/bash # Let's try with fd to find test files first and then use proper paths echo "Finding test files..." fd -e go -t f "_test.go$" echo -e "\nChecking for AWS SDK v1 imports in test files..." fd -e go -t f "_test.go$" -x rg -l "github.com/aws/aws-sdk-go/service/autoscaling" {} echo -e "\nChecking autoscalingtypes alias usage..." fd -e go -t f "_test.go$" -x rg "autoscalingtypes\." -C 2 {}Length of output: 6239
internal/service/interface.go (2)
1-8: LGTM! Clean and minimal imports.The package name and imports are well-organized, using the new AWS SDK v2 package as intended.
9-16: 🛠️ Refactor suggestionConsider adding commonly used ECS operations.
The interface covers the basic operations well. Consider adding these commonly used operations:
WaitUntilServicesStable- For deployment monitoringTagResource- For resource taggingUntagResource- For resource untaggingLet's check if these operations are used elsewhere in the codebase:
.goreleaser.yml (4)
40-41: LGTM: Snapshot configuration updated for GoReleaser v2The change from
name_templatetoversion_templateis correct according to GoReleaser v2 schema.
Line range hint
15-16: Verify package import path in ldflagsThe ldflags configuration references the package path. Let's ensure this path is correct after the upgrades.
#!/bin/bash # Description: Verify the package import path exists and is correct # Test: Check if the package path exists in go.mod rg -l "module github.com/abicky/ecsmec" go.mod # Test: Verify the cmd package exists with the revision variable ast-grep --pattern 'var revision string'
29-34: Homebrew configuration updated for GoReleaser v2The changes from
taptorepositoryandfoldertodirectoryalign with GoReleaser v2 schema. The configuration looks correct.#!/bin/bash # Description: Verify the Homebrew repository exists and is accessible # Test: Check if the Homebrew formula repository exists gh repo view abicky/homebrew-tools --json name,owner,isPrivate 2>/dev/null || echo "Repository not found"
1-1: Verify GoReleaser version compatibility with GitHub ActionsThe upgrade to GoReleaser v2 format needs to align with the GitHub Actions workflow configuration.
✅ Verification successful
GoReleaser v2 configuration is compatible with the workflow
The GitHub Actions workflow is using
goreleaser/goreleaser-action@v6, which is fully compatible with GoReleaser v2 configuration format. The workflow is properly configured with the latest version of the action.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the GitHub Actions workflow is using a compatible GoReleaser action version # Test: Check if the workflow file uses goreleaser-action v2 or higher rg -A 5 "goreleaser/goreleaser-action@v" .github/workflows/Length of output: 278
Script:
#!/bin/bash # Let's try searching for workflow files first to ensure we're looking in the right place fd "workflow" .github/ # Then let's look for any yml/yaml files in the workflows directory that might contain the goreleaser action fd "\.(yml|yaml)$" .github/workflows/ # Finally, let's check the content of any workflow files for goreleaser configuration fd "\.(yml|yaml)$" .github/workflows/ --exec cat {}Length of output: 1450
.github/workflows/main.yml (3)
18-22: LGTM! Action upgrades and Go setup improvements.The upgrades to latest stable versions of actions and the switch to
go-version-fileare good practices:
actions/checkout@v4includes performance improvements and better token handlingactions/setup-go@v5provides better caching and more reliable version resolution- Using
go-version-fileensures consistent Go versions between local and CI environments
26-26: Verify make vet implementation.The change from
go vettomake vetsuggests a custom vet configuration. Let's ensure it includes all necessary checks.✅ Verification successful
make vetimplementation is standard and sufficientThe
make vettarget simply runsgo vet ./...after generating mocks, which is the standard Go vet tool that performs static analysis of the codebase. There are no custom vet configurations or additional analyzers, making this a standard and reliable implementation for catching common mistakes in Go code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the make vet target implementation rg -A 5 "^vet:" MakefileLength of output: 165
Script:
#!/bin/bash # Check if there are any custom .vet files or additional vet configurations fd -e vet # Look for any go:vet comments or custom vet analyzer configurations rg -g '*.go' "//\s*go:vet" # Check if there are any custom vet related flags or configurations in go.mod cat go.mod # Look for any vet-related configurations in other potential build files fd "build|config" -t f -X rg "vet"Length of output: 1599
23-23: Verify mockgen installation timing.Installing mockgen in the workflow might impact CI performance. Consider moving this to a Dockerfile or including it in the project's go.mod if it's a development dependency.
go.mod (2)
19-34: LGTM: Proper indirect dependency managementThe indirect dependencies are properly tracked and versioned, particularly for AWS SDK v2 components.
6-15: Verify AWS SDK v2 migration impactThe dependency updates look good and follow best practices:
- AWS SDK v1 to v2 migration with proper service-specific modules
- Updated to stable versions of all packages
- Switched to
dario.cat/mergo(official new home)- Migrated to
go.uber.org/mock(maintained fork of golang/mock)Let's verify the AWS SDK v2 migration impact:
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have comprehensive information about the AWS SDK usage in the codebase:
AWS SDK v2 Migration Successfully Verified
The codebase has been fully migrated to AWS SDK v2:
- No imports of the v1 SDK (
github.com/aws/aws-sdk-go) were found- All AWS service clients are properly imported from v2 SDK modules
- Consistent usage of v2 types (e.g.,
ecstypes,ec2types,sqstypes)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AWS SDK v2 migration completeness # Test 1: Ensure no v1 SDK imports remain echo "Checking for any remaining v1 SDK imports..." rg "github.com/aws/aws-sdk-go" # Test 2: Verify all AWS service clients are imported from v2 echo "Verifying v2 SDK service client imports..." rg "github.com/aws/aws-sdk-go-v2/(service|config)"Length of output: 18406
internal/service/definition.go (4)
6-9: LGTM: Import changes align with AWS SDK v2 migrationThe imports have been correctly updated to use AWS SDK v2 packages, and the mergo import path has been updated to its current location.
34-34: LGTM: Correct usage of aws.Int32 for SDK v2The change to use
aws.Int32(s.DesiredCount)is the correct way to handle int32 pointers in AWS SDK v2.
14-17: Verify handling of PropagateTags edge casesThe simplified PropagateTags check might miss edge cases. The previous version likely handled nil values explicitly.
Let's check if there are any other patterns of PropagateTags usage in the codebase:
✅ Verification successful
Let me analyze the test cases to better understand the PropagateTags handling. I'll search for the complete test function.
PropagateTags handling is correctly implemented
The test cases confirm that the PropagateTags handling is appropriate:
- When PropagateTags is "NONE", it's correctly converted to empty string (test case "default")
- When PropagateTags is "TASK_DEFINITION", it's preserved as-is (test case "when PropagateTags is specified")
- There's no need for nil handling as ecstypes.Service.PropagateTags is a string type in SDK v2, not a pointer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PropagateTags usage patterns rg -A 2 "PropagateTags"Length of output: 1902
Script:
#!/bin/bash # Search for the complete test function for NewDefinitionFromExistingService ast-grep --pattern 'func TestNewDefinitionFromExistingService($$$) { $$$ }'Length of output: 5384
Line range hint
57-59: Verify type conversion compatibility with SDK v2While the type conversion should work as Definition is an alias, it's worth verifying that all fields are properly converted when using SDK v2 types.
Let's check for any field mismatches or conversion issues:
✅ Verification successful
Let me gather more information about the type definitions to verify the conversion compatibility.
From the results, I can now see that
Definitionis actually a type alias ofecs.CreateServiceInputand the conversion is used in tests with proper field access. Let me verify one final aspect about the SDK v2 compatibility.
Type conversion is safe with AWS SDK v2
The verification shows that:
Definitionis a direct type alias ofecs.CreateServiceInputfrom AWS SDK v2- The codebase consistently uses AWS SDK v2 imports (
github.com/aws/aws-sdk-go-v2/service/ecs)- The type conversion is tested and used correctly in the service creation flow
- Field access patterns in tests confirm compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CreateServiceInput usage and field access patterns ast-grep --pattern 'CreateServiceInput { $$$ }' # Search for potential type conversion issues rg "\\(\\*?ecs\\.CreateServiceInput\\)"Length of output: 157
Script:
#!/bin/bash # Check Definition struct fields ast-grep --pattern 'type Definition struct { $$$ }' # Check CreateServiceInput usage in tests or examples rg -A 5 "CreateServiceInput" # Look for any type conversion related tests rg -l "Test.*CreateService"Length of output: 3104
Script:
#!/bin/bash # Check AWS SDK v2 imports rg "github.com/aws/aws-sdk-go-v2/service/ecs" # Look for any Definition usage patterns rg "type Definition" -A 3Length of output: 2074
internal/service/definition_test.go (3)
7-8: LGTM: Clean SDK v2 import migrationThe imports have been correctly updated to use AWS SDK v2 packages, with a clear type alias for ECS types that improves code readability.
16-16: LGTM: Clean type migration to SDK v2The test struct definition has been properly updated to use the new
ecstypes.Servicetype from AWS SDK v2.
Line range hint
77-83: LGTM: Clear test assertionsThe test assertions use appropriate deep equality checks and provide clear error messages that will help debug any failures.
internal/capacity/interface.go (3)
1-10: LGTM! Clean package structure and imports.The package name and imports are well-organized, using only the necessary AWS SDK v2 packages.
20-26: Verify error handling for Spot Fleet operations.While the interface is well-defined, Spot Fleet operations (especially
ModifySpotFleetRequest) might need special error handling in the implementations due to potential capacity or pricing constraints.Consider documenting common error scenarios and handling strategies for Spot Fleet operations in the implementation. The following script will help identify existing error handling patterns in the codebase:
38-41: Consider partial failure handling for DeleteMessageBatch.The
DeleteMessageBatchoperation can result in partial failures where some messages are deleted while others fail.Ensure the implementation handles partial batch failures appropriately. The following script will help identify existing batch operation handling patterns:
cmd/root.go (3)
4-4: Importing the 'context' package is appropriateThe addition of the
"context"import is necessary for the updated functions that require acontext.Contextparameter.
7-8: Updated AWS SDK imports to v2The import paths have been correctly updated to use AWS SDK for Go v2, aligning with the migration from v1 to v2.
35-35: Ensure Go version compatibility with 'any' keywordThe use of
anyas a type alias forinterface{}requires Go 1.18 or later. Please verify that the Go module'sgo.modfile specifiesgo 1.18or higher to maintain compatibility.Run the following script to check the Go version specified in
go.mod:✅ Verification successful
Go version requirement is satisfied
The
go.modfile specifies Go version 1.23.2, which is well above the minimum required version (1.18) for using theanykeyword. The code is compatible with the project's Go version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check that the go.mod file specifies Go version 1.18 or higher # Expected output: Go version line with version >=1.18 # Extract the Go version from go.mod grep '^go ' go.modLength of output: 30
cmd/terminatespotfleetinstances.go (3)
4-5: Approved: Updated AWS SDK imports to v2The imports for
ec2andecshave been correctly updated to use the AWS SDK v2 packages. This aligns with the PR objective to upgrade to AWS SDK v2.
29-29: ConfirmbatchSizetype change fromInt64toInt32The
batch-sizeflag type has been changed fromInt64toInt32, andGetInt32is used to retrieve its value. Please verify that this type change is consistent with the AWS SDK v2 requirements and that all functions utilizingbatchSizeare updated accordingly to acceptint32.Also applies to: 37-37
54-54: Approved: Added context toTerminateAllInstancesmethod callPassing
cmd.Context()tosfr.TerminateAllInstancesis appropriate and adheres to best practices for context propagation in Go.internal/capacity/poller.go (8)
7-9: LGTM: Updated imports to AWS SDK v2The imports have been correctly updated to use the AWS SDK v2 packages, ensuring compatibility with the new version.
16-17: Interface methods updated with context and AWS SDK v2 typesThe
Pollerinterface methods have been appropriately updated to includecontext.Contextparameters and to use AWS SDK v2 types. This ensures proper context management and type consistency across the codebase.
25-29: Constructor updated to acceptSQSAPIinterfaceThe
NewSQSQueuePollerfunction now accepts theSQSAPIinterface as a parameter, aligning with the updatedsqsSvcfield type. This change promotes abstraction and facilitates testing with mock implementations.
32-43: MethodPollupdated with context parameter and AWS SDK v2 typesThe
Pollmethod has been correctly updated to include thecontext.Contextparameter and to use AWS SDK v2 types. This enhances context management and ensures proper cancellation and timeout handling.
38-40: Proper propagation of context inPollmethodThe context is properly passed to the
PollOncemethod within thePollloop, ensuring that cancellation signals are respected during long polling operations.
45-45: MethodPollOncesignature updated appropriatelyThe
PollOncemethod includes thecontext.Contextparameter and updates thewaitTimeSecondsparameter toint32, matching the expected types in AWS SDK v2'sReceiveMessageInput.
65-68: UpdatedDeleteMessageBatchcall with contextThe
DeleteMessageBatchmethod now correctly accepts the context parameter, ensuring consistency with AWS SDK v2's API and proper context propagation.
22-22: Verify the definition and usage ofSQSAPIinterfaceThe
sqsSvcfield's type has been changed toSQSAPI. Ensure that theSQSAPIinterface is properly defined and includes all the necessary methods required bySQSQueuePoller.Run the following script to locate the definition of
SQSAPIand display its methods:✅ Verification successful
Let me verify how these methods are used in the
SQSQueuePollerto ensure the interface provides all necessary methods.
Let me try with ripgrep to find the usage of sqsSvc:
SQSAPI interface is properly defined and matches usage
The
SQSAPIinterface is correctly defined with exactly the two methods required bySQSQueuePoller:
ReceiveMessage- used to poll for new messagesDeleteMessageBatch- used to remove processed messagesThe interface definition in
internal/capacity/interface.goperfectly matches its usage ininternal/capacity/poller.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the definition of the SQSAPI interface and list its methods. # Expectation: The SQSAPI interface should be defined and include methods like ReceiveMessage and DeleteMessageBatch. # Search for the SQSAPI interface definition. rg -A 10 -B 2 'type SQSAPI interface' # Alternatively, use ast-grep if available. # ast-grep --pattern 'type SQSAPI interface { # $$$ # }'Length of output: 500
Script:
#!/bin/bash # Search for usage of sqsSvc within SQSQueuePoller ast-grep --pattern 'func ($_ *SQSQueuePoller) $$$() { $$$ $_.sqsSvc.$$$ $$$ }'Length of output: 86
Script:
#!/bin/bash # Search for usage of sqsSvc in the poller file rg -A 5 -B 5 'sqsSvc\.' internal/capacity/poller.goLength of output: 735
cmd/replaceautoscalinggroupinstances.go (7)
4-6: Imports updated to AWS SDK v2The import statements have been correctly updated to use AWS SDK for Go v2 packages, facilitating the migration to the newer SDK version.
39-39: Consistent retrieval ofbatch-sizeasInt32Retrieving the
batch-sizeflag usingGetInt32aligns with the updated flag type. Ensure that all variables and functions that usebatchSizeare updated toint32accordingly.
41-44: Initialization of AWS config with context is appropriateUsing
newConfig(cmd.Context())enhances context management and aligns with AWS SDK for Go v2 best practices, promoting better control over request lifecycles.
46-49: AWS service clients instantiated withNewFromConfigCreating service clients using
autoscaling.NewFromConfig(cfg)andec2.NewFromConfig(cfg)is the correct approach in AWS SDK v2, ensuring that clients are configured properly.
56-56: EnsureReplaceInstancesmethod handles context properlyPassing
cmd.Context()toReplaceInstancespromotes better context management. Verify that theReplaceInstancesmethod inAutoScalingGroupacceptscontext.Contextas the first parameter and utilizes it correctly within the method.#!/bin/bash # Description: Verify that 'ReplaceInstances' method accepts 'context.Context' and uses it appropriately. # Test 1: Search for the method definition accepting context. ast-grep --pattern $'func ($_).ReplaceInstances(ctx context.Context, $$$) error { $$$ }' # Test 2: Ensure all calls to 'ReplaceInstances' pass a context. ast-grep --pattern $'$_ := $_.ReplaceInstances($_, $$$)'
51-54: VerifyNewDrainerfunction signature matches updated usageThe
NewDrainerfunction is now called withcluster,batchSize, andecs.NewFromConfig(cfg). Verify that theNewDrainerfunction has been updated to accept these parameters with the expected types.#!/bin/bash # Description: Confirm that the 'NewDrainer' function signature accepts (string, int32, *ecs.Client). # Test: Search for the 'NewDrainer' function definition with the updated parameters. ast-grep --pattern $'func NewDrainer(cluster string, batchSize int32, ecsClient *ecs.Client) ($_, error) { $$$ }'
31-31: Verify 'batch-size' flag type change fromInt64toInt32The
batch-sizeflag's data type has been changed fromInt64toInt32. Ensure that this change does not introduce any issues, especially if values exceeding theInt32range were previously expected.✅ Verification successful
Change from Int64 to Int32 for batch-size flag is safe and consistent
The verification shows that:
- The default value
MaxListableContainerInstancesis defined as 100 ininternal/const/ecsconst/const.go- There's a validation in
internal/capacity/drainer.gothat ensures batch size cannot exceed 100- All usages of the flag consistently use
GetInt32()- No Int64 values or large batch sizes are used anywhere in the codebase
Since the maximum allowed value is 100, which is well within Int32 range, the type change from Int64 to Int32 is safe and won't cause any issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no values exceeding Int32 are used with 'batch-size' and that all usages are consistent. # Test 1: Search for all retrievals of 'batch-size' and ensure they use GetInt32. ast-grep --pattern $'$_ := $_.Flags().GetInt32("batch-size")' # Test 2: Check for any retrievals using GetInt64, which may indicate inconsistencies. ast-grep --pattern $'$_ := $_.Flags().GetInt64("batch-size")' # Test 3: Search for any hardcoded values assigned to 'batchSize' that exceed Int32 limits. rg -e 'batchSize\s*=\s*\d{10,}' -A 2Length of output: 308
Script:
#!/bin/bash # Let's try a different approach to verify the batch-size flag usage # Check for any references to batch-size flag rg -i "batch-size" -A 2 -B 2 # Look for any Int64 type usage with batch rg -i "int64.*batch" -A 2 -B 2 # Look for batchSize variable declarations and usages ast-grep --pattern 'batchSize := $_' # Check the constant value being used as default rg "MaxListableContainerInstances"Length of output: 3679
cmd/recreateservice.go (2)
7-7: Approved: Updated import to AWS SDK v2The import statement has been correctly updated to use AWS SDK for Go v2.
72-74: Verify theRecreatemethod signature in the service packageThe call to
Recreatenow includescmd.Context()as the first argument. Ensure that theRecreatemethod in theservicepackage has been updated to accept and properly utilize thecontext.Contextparameter.Run the following script to confirm that the
Recreatemethod acceptscontext.Context:✅ Verification successful
The
Recreatemethod correctly accepts and utilizes the context parameterThe verification confirms that the
Recreatemethod ininternal/service/service.gohas the correct signaturefunc (s *Service) Recreate(ctx context.Context, cluster string, serviceName string, overrides Definition) errorand properly propagates the context to its internal operations (copyandstopAndDelete).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Recreate method in the service package accepts context.Context # Test: Search for the Recreate method definition that includes context.Context # Expect: The Recreate method signature starts with ctx context.Context ast-grep --pattern $'func ($_) Recreate(ctx context.Context, $$$)'Length of output: 2002
internal/capacity/spotfleetrequest.go (4)
18-22: Consider embeddingSpotFleetRequestConfigfor cleaner struct initialization.By embedding
ec2types.SpotFleetRequestConfig, you can access its fields directly without specifying the field name, which improves code readability.
148-149: Pass context toreloadmethod consistently.Ensure that all calls to
reloadpass the appropriate context. With the earlier suggestion to modifyNewSpotFleetRequest, this will maintain consistency.
103-108: Verify thatModifySpotFleetRequestis correctly setting the new target capacity.Ensure that the
ModifySpotFleetRequestInputis correctly constructed and that the new target capacity is being set as intended.Run the following script to check for correct usage:
#!/bin/bash # Description: Verify that ModifySpotFleetRequest is properly used. # Search for usages of ModifySpotFleetRequest and examine the parameters. rg -A 5 'ModifySpotFleetRequest\('
60-65: Update the usage ofInstanceTerminatedWaiter.The creation of the waiter for instance termination may not align with the AWS SDK for Go v2 patterns. Ensure that the waiter is correctly instantiated and utilized according to the SDK's guidelines.
Run the following script to verify the correct usage of
InstanceTerminatedWaiter:✅ Verification successful
The
InstanceTerminatedWaiterusage is correct and follows AWS SDK v2 patternsThe implementation correctly:
- Uses the AWS SDK Go v2 package
github.com/aws/aws-sdk-go-v2/service/ec2- Creates the waiter with proper initialization using
NewInstanceTerminatedWaiter- Configures waiter options through the provided functional options pattern
- Uses consistent waiter patterns across the codebase (same usage in both
spotfleetrequest.goandautoscalinggroup.go)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct instantiation and usage of InstanceTerminatedWaiter in the codebase. # Search for the usage of InstanceTerminatedWaiter. rg -A 5 'ec2\.NewInstanceTerminatedWaiter\('Length of output: 1864
Script:
#!/bin/bash # Let's search for the AWS SDK v2 waiter patterns in the codebase rg -A 5 'NewInstanceTerminatedWaiter|InstanceTerminatedWaiter' # Also check imports to verify SDK version rg '"github.com/aws/aws-sdk-go-v2/service/ec2"'Length of output: 3771
internal/service/service.go (8)
93-99: Ensure proper timeout and max delay settings for service stability waiterReview the timeout and max delay values used in
ecs.NewServicesStableWaiter. Ensure that10*time.Minuteis sufficient for your services to become stable and adjusto.MaxDelayif necessary.Verify the waiter's configuration to ensure it aligns with your service expectations.
Line range hint
56-69: Handle missing or inactive services gracefully incopymethodWhen the service does not exist or is not active, the function returns an error. Ensure that the calling functions handle these errors appropriately.
Check that the errors from
s.copyare properly handled upstream.
27-53: Propagate context consistently throughout methodsEnsure that the
ctxparameter is passed to all methods and external calls withinRecreateto handle cancellation and timeouts effectively.
88-105: Confirm proper usage of AWS SDK v2 waiter for service stabilityThe usage of
ecs.NewServicesStableWaiterseems appropriate. Verify that it works as expected with your service configurations.
21-25: InitializeServicestruct with newECSAPIinterfaceThe change to use
ECSAPIaligns with the AWS SDK v2 upgrade. Good job updating the constructor accordingly.
122-136: Collect all running task ARNs before updating desired countThe code correctly collects all running tasks before setting the desired count to zero. This ensures that all tasks are accounted for when waiting for them to stop.
Line range hint
55-86: Updatecopymethod for AWS SDK v2 compatibilityThe
copymethod has been appropriately updated to use AWS SDK v2, including the use of context and updated input types.
147-154: Confirm task chunking logic instopAndWaitUntilStoppedConfirm that the chunking of
taskArnsusingslices.Chunkworks as intended, especially with large numbers of tasks. This ensures that all tasks are properly waited on without exceeding API limits.You can verify this by reviewing the chunking logic:
cmd/reduceclustercapacity.go (23)
4-4: Importing 'context' package is appropriateThe addition of the
"context"package is necessary for passing context to AWS SDK v2 methods.
8-15: Update AWS SDK imports to V2Updating the AWS SDK imports to use V2 is appropriate for migrating from V1 to V2.
57-57: Retrieve 'amount' flag as Int32Retrieving the 'amount' flag as
Int32aligns with its definition and ensures consistency.
63-63: Initialize configuration with contextUsing
newConfig(cmd.Context())appropriately initializes the AWS configuration with context support.
68-68: Create ECS client with updated SDKInitializing the ECS client with
ecs.NewFromConfig(cfg)is correct for AWS SDK V2.
74-74: Create Auto Scaling client with updated SDKUsing
autoscaling.NewFromConfig(cfg)andec2.NewFromConfig(cfg)correctly creates clients compatible with AWS SDK V2.
79-79: Pass context to 'ReduceCapacity' methodIncluding
cmd.Context()when callingReduceCapacityensures proper context propagation.
83-83: Initialize Spot Fleet client with updated SDKCreating the Spot Fleet Request with
ec2.NewFromConfig(cfg)is appropriate for AWS SDK V2.
88-88: Create SQS client with updated SDKUsing
sqs.NewFromConfig(cfg)correctly initializes the SQS client for AWS SDK V2.
89-89: Create SQS queue with contextCalling
putSQSQueuewithcmd.Context(),sqsSvc, and the queue name is appropriate for context-aware operations.
94-94: Create EventBridge client with updated SDKUsing
eventbridge.NewFromConfig(cfg)is correct for initializing the EventBridge client with AWS SDK V2.
96-96: Set up EventBridge rule with contextCalling
putEventRulewith the necessary parameters and context is appropriate.
100-100: Pass context to 'ReduceCapacity' methodIncluding
cmd.Context()when callingsfr.ReduceCapacityensures proper context management for the Spot Fleet Request.
104-104: Delete EventBridge rule with contextCalling
deleteEventRulewithcmd.Context()ensures that the deletion operation respects context deadlines and cancellations.
107-107: Delete SQS queue with contextCalling
deleteSQSQueuewithcmd.Context()is appropriate for context-aware cleanup operations.
115-116: Define 'putSQSQueue' function with contextDefining
putSQSQueueto acceptcontext.Contextand using it when calling AWS SDK methods is appropriate for handling timeouts and cancellations.
123-125: Retrieve 'QueueArn' attribute with correct typesUsing
[]sqstypes.QueueAttributeName{"QueueArn"}ensures that the correct attribute types are requested.
136-137: Define 'deleteSQSQueue' function with contextDefining
deleteSQSQueueto acceptcontext.Contextand using it when calling AWS SDK methods is appropriate.
143-144: Define 'putEventRule' function with contextDefining
putEventRuleto acceptcontext.Contextand using it when interacting with AWS services ensures proper context handling.
Line range hint
152-171: Set SQS queue attributes with appropriate policySetting the queue policy using
SetQueueAttributesensures that EventBridge can send messages to the SQS queue securely.
179-181: Add targets to EventBridge ruleUsing
PutTargetsto add the SQS queue as a target for the EventBridge rule is correct and ensures proper event routing.
195-197: Define 'deleteEventRule' function with contextDefining
deleteEventRuleto acceptcontext.Contextand using it when removing targets and deleting the rule is appropriate for context-aware operations.
204-205: Delete EventBridge rule with force optionUsing
Force: truewhen deleting the EventBridge rule ensures it is deleted even if it has associated targets.internal/service/service_test.go (13)
4-5: Imports updated for context managementThe addition of the
"context"and"io"packages is appropriate for handling context operations and discarding log outputs in tests.
11-14: AWS SDK v2 imports correctly integratedThe import paths have been updated to reflect AWS SDK for Go v2, and the alias
ecstypesis appropriately used for ECS types.
17-17: Mock package import updatedImporting
github.com/abicky/ecsmec/internal/testing/servicemockensures that the mocks are aligned with the updated internal structure.
22-22: Logs suppressed during tests for cleaner outputSetting
log.SetOutput(io.Discard)inTestMaineffectively suppresses log outputs during testing, resulting in cleaner test outputs.
28-28: Added context parameter toexpectCopyfunctionIncluding
ctx context.Contextas a parameter inexpectCopyaligns with the context-aware patterns introduced in AWS SDK v2.
37-47: Context passed explicitly in mock expectationsIn the
expectCopyfunction, passingctxexplicitly toecsMock.EXPECT().DescribeServicesenhances clarity and aligns with best practices.
Line range hint
54-67: CreateService mock updated with contextPassing
ctxtoecsMock.EXPECT().CreateServiceensures that the context is properly handled in the mock expectations.
90-91: Added context parameter toexpectStopAndDeletefunctionIncluding
ctx context.ContextinexpectStopAndDeleteensures that the function adheres to context-aware practices.
102-108: Context passed explicitly inListTasksmockPassing
ctxtoecsMock.EXPECT().ListTasksmaintains consistency across mock expectations.
173-174: Context initialized properly in test caseCreating a background context with
ctx := context.Background()prepares the context for use in subsequent function calls.
179-182: Mocks called with context in test sequenceMock functions are called with
ctx, ensuring that context is consistently passed throughout the test sequence.
196-197: Context initialized in new test caseInitializing
ctxin the "with overriding service name" test case maintains consistency across different test scenarios.
235-238: Mocks set up correctly for exception testsSetting up
ecsMockand mockingDescribeServiceswithctxin exception tests ensures proper test coverage for error scenarios.internal/capacity/drainer.go (2)
8-8: Verify the Go version compatibility with theslicespackageThe
slicespackage was introduced in Go 1.21. Please ensure that the project is using Go 1.21 or a later version to avoid build issues.Run the following script to confirm the Go version specified in
go.mod:✅ Verification successful
Go version is compatible with the
slicespackageThe project uses Go 1.23.2, which fully supports the
slicespackage introduced in Go 1.21.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the Go version specified in go.mod # Expected: Go version should be 1.21 or higher grep '^go 1\.[0-9]*' go.modLength of output: 39
228-228:⚠️ Potential issueCorrect the filter syntax for instance IDs in the AWS SDK filter expression
AWS filter expressions require string values to be enclosed in single quotes. Without quoting, instance IDs containing commas may cause unexpected behavior.
Apply this diff to properly quote the instance IDs:
- Filter: aws.String(fmt.Sprintf("ec2InstanceId in [%s]", strings.Join(instanceIDs, ","))), + quotedInstanceIDs := make([]string, len(instanceIDs)) + for i, id := range instanceIDs { + quotedInstanceIDs[i] = fmt.Sprintf("'%s'", id) + } + Filter: aws.String(fmt.Sprintf("ec2InstanceId in [%s]", strings.Join(quotedInstanceIDs, ","))),Likely invalid or redundant comment.
internal/capacity/drainer_test.go (11)
4-4: Importingcontextpackage correctlyThe
contextpackage is imported appropriately to support context management with AWS SDK v2.
9-13: AWS SDK imports updated to v2 successfullyThe AWS SDK imports have been correctly updated to use the v2 version, and the type aliases (
ecstypes,sqstypes) are appropriately defined.
16-16: Mock package import adjusted appropriatelyThe import of
capacitymockreflects the updated path for the mock implementations, which is consistent with the changes in the codebase.
24-24: Context initialized for test casesInitializing
ctxwithcontext.Background()sets up the context for the subsequent AWS SDK calls in the test.
161-161: Context initialized for test casesInitializing
ctxwithcontext.Background()prepares the context for AWS SDK calls in this test.
54-63: Ensure correct usage of AWS SDK v2 methods and parametersThe call to
ecsMock.EXPECT().ListTasksand subsequent handling aligns with AWS SDK v2 requirements, including context and options parameters.
79-87: Validate task stopping logicThe stopping of tasks uses the updated method signatures and seems appropriately adapted to AWS SDK v2.
88-111: Correct use of AWS SDK v2 waiters and service descriptionsThe implementation of waiters and service description methods reflects the updated AWS SDK v2 patterns.
206-216: Consistency in handling task listingsThe updates to
ListTasksmethod calls include the correct usage of context and options, ensuring consistent behavior across the test cases.
262-263:⚠️ Potential issueInitialize context in the test function
In this test case,
ctxis declared but not initialized. Please initializectxbefore using it.Apply this diff:
- ctx := context.Background() + + ctx := context.Background()Likely invalid or redundant comment.
127-128:⚠️ Potential issueInitialize context in the test function
In this test case,
ctxis declared but not initialized. Ensure thatctxis initialized before usage.Apply this diff to initialize
ctx:- ctx := context.Background() + + ctx := context.Background()Likely invalid or redundant comment.
internal/capacity/spotfleetrequest_test.go (10)
8-16: Imports correctly updated to AWS SDK v2The import statements have been successfully updated to use the AWS SDK for Go v2, including the correct paths for
aws,ec2, and their respective types. This ensures compatibility with the upgraded SDK.
39-40: Context handling added appropriatelyThe addition of
ctx := context.Background()establishes a context for the subsequent method calls, which is essential for the AWS SDK v2 operations.
45-46: Method calls updated with context parameterAll mocked method calls now include the
ctxparameter, aligning with the AWS SDK v2 requirements. This ensures that context is properly propagated through the calls.Also applies to: 57-59, 62-64, 76-103
111-113: Test assertion correctly checks for no errorsThe test now correctly checks if
sfr.TerminateAllInstances(ctx, drainerMock)returns an error and reports if it does. This ensures the test accurately validates the expected behavior.
117-120: Context and mocks initialized correctly for new test caseThe context and mock objects are appropriately initialized for the "with the state active" test case, ensuring isolation and correctness of the test environment.
146-148: Test assertion correctly checks for expected errorThe test now checks that an error is returned when
sfr.TerminateAllInstances(ctx, drainerMock)is called in an invalid state, which is the correct expected behavior.
156-158: UpdateTargetCapacityfields toint32The
finalTargetCapacityandamountfields have been changed fromint64toint32to match the AWS SDK v2 types. Ensure that any capacity values used do not exceed theint32limit to prevent potential overflow issues.
235-236: Context initialized for each test caseContexts are correctly initialized for each test case using
context.Background(), ensuring that each test runs with the appropriate context.Also applies to: 283-284, 380-381
238-240: Mocks initialized appropriatelyMock objects (
ec2Mock,drainerMock,pollerMock) are correctly initialized for each test case, ensuring that dependencies are properly mocked.Also applies to: 286-288, 383-385
265-265: Drainer mock process updated with contextThe
drainerMock.EXPECT().ProcessInterruptions(ctx, messages)now includes thectxparameter, ensuring consistency with the method's signature and context propagation.internal/capacity/autoscalinggroup_test.go (2)
4-6: Confirm the availability of theslicespackageThe
slicespackage introduced in line6~is part of the Go standard library starting from Go 1.18. Ensure that the project’s Go version is 1.18 or higher to use this package.Run the following script to confirm the Go version:
✅ Verification successful
Go version requirement is satisfied
The project uses Go 1.23.2, which is well above the minimum required version (1.18) for using the
slicespackage from the standard library.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the Go version used in the project. # Expected: The Go version should be 1.18 or higher. # Check the Go version specified in `go.mod` or any configuration files. rg -A 2 '^go ' go.modLength of output: 44
712-712:⚠️ Potential issueUse
ctxinstead ofcontext.Background()for consistent context propagationThe call to
drainerMock.EXPECT().Drainusescontext.Background()instead of thectxvariable. Replacecontext.Background()withctxto ensure consistent context propagation throughout the test.Apply this diff to fix the issue:
drainerMock.EXPECT().Drain(context.Background(), gomock.Len(len(instancesToTerminate))),should be:
drainerMock.EXPECT().Drain(ctx, gomock.Len(len(instancesToTerminate))),Likely invalid or redundant comment.
| if int32(len(sortedInstanceIDs)) == count { | ||
| break Loop | ||
| } | ||
| var i *ec2.Instance | ||
| var i ec2types.Instance |
There was a problem hiding this comment.
Ensure Non-Empty Slices Before Indexing
When extracting the first instance from azToInstances[az], there is a risk of index out-of-range errors if the slice is empty. To prevent a potential panic, ensure the slice is not empty before accessing it.
Consider adding a check before indexing:
for {
for _, az := range azs {
+ if len(azToInstances[az]) == 0 {
+ continue
+ }
if int32(len(sortedInstanceIDs)) == count {
break Loop
}
var i ec2types.Instance
i, azToInstances[az] = azToInstances[az][0], azToInstances[az][1:]
sortedInstanceIDs = append(sortedInstanceIDs, *i.InstanceId)
}
}This ensures that you only access the slice when it contains elements, avoiding runtime errors.
Committable suggestion was skipped due to low confidence.
| asg.AutoScalingGroup = resp.AutoScalingGroups[0] | ||
| asg.OriginalDesiredCapacity = asg.DesiredCapacity | ||
| asg.OriginalMaxSize = asg.MaxSize | ||
| for _, t := range asg.Tags { |
There was a problem hiding this comment.
Check for Empty AutoScalingGroups Response
Accessing resp.AutoScalingGroups[0] without verifying that the slice has elements can lead to a runtime panic if the slice is empty. To avoid potential out-of-bounds errors, ensure that the response contains at least one Auto Scaling Group before accessing it.
Apply this diff to fix the issue:
if err != nil {
return xerrors.Errorf("failed to describe the auto scaling group: %w", err)
}
-if resp.AutoScalingGroups == nil {
+if resp.AutoScalingGroups == nil || len(resp.AutoScalingGroups) == 0 {
return xerrors.Errorf("the auto scaling group \"%s\" doesn't exist", asg.name)
}
asg.AutoScalingGroup = resp.AutoScalingGroups[0]This change adds a length check to ensure safe access to the slice.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Thank you for pointing it out! I've fixed in 26e00d0.
Because github.com/golang/mock/gomock is no longer maintained.
darccio/mergo#175 has already been merged.
Upgraded cobra by "go get -u github.com/spf13/cobra && go mod tidy"
by "go get go@latest"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/recreateservice.go (1)
72-74: Consider enhancing the error message with service detailsWhile the code correctly uses the AWS SDK v2 patterns, the error message could be more helpful by including the service name and cluster in the error message.
Apply this diff to improve the error message:
- return newRuntimeError("failed to recreate the service: %w", err) + return newRuntimeError("failed to recreate service '%s' in cluster '%s': %w", serviceName, cluster, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Makefile (2 hunks)
- cmd/recreateservice.go (2 hunks)
- internal/service/service.go (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
📓 Learnings (1)
internal/service/service.go (1)
Learnt from: abicky PR: abicky/ecsmec#1 File: internal/service/service.go:177-183 Timestamp: 2024-10-28T07:16:19.928Z Learning: In Go 1.22 and later, the syntax `for i := range tries` allows looping over an integer variable.
🔇 Additional comments (5)
cmd/recreateservice.go (3)
7-7: LGTM: AWS SDK v2 import updateThe import statement has been correctly updated to use the AWS SDK v2 package.
67-69: LGTM: Proper AWS SDK v2 configuration initializationThe code correctly initializes the AWS configuration using the new SDK v2 pattern and properly propagates the context from the cobra command.
Line range hint
7-74: Verify AWS SDK v2 migration completenessLet's ensure the AWS SDK v2 migration is consistent across the codebase.
✅ Verification successful
Let me gather more information to verify the AWS SDK migration and context usage.
Based on the gathered information, I can now provide the final response:
AWS SDK v2 Migration and Context Usage Verified
The codebase consistently uses AWS SDK v2 throughout:
- All imports are from
aws-sdk-go-v2packages- No v1 SDK imports were found
- Context is properly propagated in service operations, with
ctxbeing passed through from the cobra command to all AWS service calls🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining v1 SDK imports echo "Checking for remaining AWS SDK v1 imports..." rg "github.com/aws/aws-sdk-go/service" # Check for consistent context usage in service operations echo "Checking context usage in service operations..." ast-grep --pattern 'func ($x *Service) Recreate($$$) { $$$ }'Length of output: 308
Script:
#!/bin/bash # Check all AWS SDK imports to ensure v2 consistency echo "Checking all AWS SDK imports..." rg "github.com/aws/aws-sdk-go" rg "github.com/aws/aws-sdk-go-v2" # Check context usage in service-related files echo "Checking service implementations..." fd service.go --exec cat {}Length of output: 29846
internal/service/service.go (2)
12-12: Replace deprecatedgolang.org/x/xerrorswith standarderrorsandfmtpackagesSince Go 1.13, the standard library's
errorsandfmtpackages provide error wrapping and unwrapping functionalities. Updating to these packages simplifies dependencies and follows current best practices.
182-183: Update error handling to match AWS SDK v2 conventionsThe error type
*ecstypes.InvalidParameterExceptionmay not be accurate in AWS SDK v2. Additionally,e.ErrorMessage()might have changed in the new SDK version. Please verify and update the error handling to align with AWS SDK v2 conventions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Because the document of AddInt32 and LoadInt32 recommends using it: > Consider using the more ergonomic and less error-prone Int32.Add > instead. > Consider using the more ergonomic and less error-prone Int32.Load > instead.
abicky
force-pushed
the
upgrade-go-and-packages
branch
from
12302e9
to
0401bc1
Compare
abicky
force-pushed
the
upgrade-go-and-packages
branch
from
0401bc1
to
9a92aaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (14)
cmd/root.go (1)
Line range hint
36-40: Good use ofanytype, but consider updating error creation.The update to use
anyinstead ofinterface{}is a good modernization. However, to align with the previous suggestion:func newRuntimeError(format string, a ...any) error { return &runtimeError{ - err: xerrors.Errorf(format, a...), + err: fmt.Errorf(format, a...), } }cmd/replaceautoscalinggroupinstances.go (1)
41-56: LGTM: AWS SDK v2 client initialization is properly implemented.The changes correctly implement the AWS SDK v2 patterns:
- Using configuration-based client initialization
- Proper context propagation
- Consistent error handling
However, consider using
fmt.Errorfwith%winstead of the customnewRuntimeErrorwrapper for better error handling clarity.Consider this improvement:
- return newRuntimeError("failed to initialize a session: %w", err) + return fmt.Errorf("failed to initialize a session: %w", err)internal/service/definition_test.go (1)
35-73: Consider adding test cases with different DesiredCount valuesWhile the test cases cover various scenarios for roles and tag propagation, they all use the same
DesiredCountvalue of 1. Consider adding test cases with differentDesiredCountvalues to ensure the behavior is correct across various scenarios.Example additional test case:
+ { + name: "when DesiredCount is zero", + s: ecstypes.Service{ + ClusterArn: aws.String("arn:aws:ecs:ap-northeast-1:123456789:cluster/default"), + DesiredCount: 0, + PropagateTags: ecstypes.PropagateTagsNone, + RoleArn: aws.String("arn:aws:iam::123456789:role/CustomRole"), + }, + want: &service.Definition{ + Cluster: aws.String("arn:aws:ecs:ap-northeast-1:123456789:cluster/default"), + DesiredCount: aws.Int32(0), + Role: aws.String("arn:aws:iam::123456789:role/CustomRole"), + }, + },internal/capacity/interface.go (2)
12-18: Well-structured interface following AWS SDK v2 patterns.The interface provides a clean abstraction for Auto Scaling operations. Consider splitting this interface if it grows larger to maintain the Interface Segregation Principle (ISP).
28-36: LGTM! Consider grouping related operations.The interface provides comprehensive ECS operations coverage. Consider grouping related operations into smaller interfaces for better maintainability:
type ECSTaskAPI interface { DescribeTasks(context.Context, *ecs.DescribeTasksInput, ...func(*ecs.Options)) (*ecs.DescribeTasksOutput, error) ListTasks(context.Context, *ecs.ListTasksInput, ...func(*ecs.Options)) (*ecs.ListTasksOutput, error) StopTask(context.Context, *ecs.StopTaskInput, ...func(*ecs.Options)) (*ecs.StopTaskOutput, error) } type ECSContainerInstanceAPI interface { DescribeContainerInstances(context.Context, *ecs.DescribeContainerInstancesInput, ...func(*ecs.Options)) (*ecs.DescribeContainerInstancesOutput, error) ListContainerInstances(context.Context, *ecs.ListContainerInstancesInput, ...func(*ecs.Options)) (*ecs.ListContainerInstancesOutput, error) UpdateContainerInstancesState(context.Context, *ecs.UpdateContainerInstancesStateInput, ...func(*ecs.Options)) (*ecs.UpdateContainerInstancesStateOutput, error) } type ECSServiceAPI interface { DescribeServices(context.Context, *ecs.DescribeServicesInput, ...func(*ecs.Options)) (*ecs.DescribeServicesOutput, error) } type ECSAPI interface { ECSTaskAPI ECSContainerInstanceAPI ECSServiceAPI }internal/capacity/spotfleetrequest.go (2)
44-45: Use SDK constants in error message for better maintainability.Consider using the SDK's string constants for fleet types in the error message to ensure consistency with SDK changes.
- return xerrors.Errorf("the spot fleet request with the type \"%s\" must be canceled, but the state is \"%s\"", ec2types.FleetTypeMaintain, sfr.SpotFleetRequestState) + return xerrors.Errorf("the spot fleet request with the type %q must be canceled, but the state is %q", ec2types.FleetTypeMaintain, sfr.SpotFleetRequestState)
Line range hint
162-193: Consider extracting duplicate weighted capacity validation logic.The weighted capacity validation logic is duplicated between LaunchTemplateConfigs and LaunchSpecifications sections. Consider extracting this into a helper function.
+func validateWeightedCapacity(value *float64, currentCapacity int32) (int32, error) { + if value == nil { + return currentCapacity, nil + } + if *value != float64(int32(*value)) { + return 0, xerrors.Errorf("currently float weighted capacities are not supported") + } + newCapacity := int32(*value) + if currentCapacity != 0 && currentCapacity != newCapacity { + return 0, xerrors.Errorf("currently mixed weighted capacities are not supported") + } + return newCapacity, nil +} func (sfr *SpotFleetRequest) weightedCapacity() (int32, error) { - weightedCapacity := int32(0) + var weightedCapacity int32 // Spot Fleet Requests with a LaunchTemplate for _, conf := range sfr.SpotFleetRequestConfigData.LaunchTemplateConfigs { for _, o := range conf.Overrides { - if *o.WeightedCapacity != float64(int32(*o.WeightedCapacity)) { - return 0, xerrors.Errorf("currently float weighted capacities are not supported") - } - if weightedCapacity == 0 { - weightedCapacity = int32(*o.WeightedCapacity) - } - if weightedCapacity != int32(*o.WeightedCapacity) { - return 0, xerrors.Errorf("currently mixed weighted capacities are not supported") + var err error + if weightedCapacity, err = validateWeightedCapacity(o.WeightedCapacity, weightedCapacity); err != nil { + return 0, err } } }internal/service/service.go (1)
94-100: Consider making timeout values configurableThe waiter configuration uses hardcoded values for
MaxDelay(15s) and timeout (10m). Consider making these configurable to support different service deployment scenarios.+type ServiceConfig struct { + WaiterMaxDelay time.Duration + WaiterTimeout time.Duration +} -func NewService(ecsSvc ECSAPI) *Service { +func NewService(ecsSvc ECSAPI, config *ServiceConfig) *Service { return &Service{ ecsSvc, + config: config, } } func (s *Service) createAndWaitUntilStable(ctx context.Context, config *ecs.CreateServiceInput) error { waiter := ecs.NewServicesStableWaiter(s.ecsSvc, func(o *ecs.ServicesStableWaiterOptions) { - o.MaxDelay = 15 * time.Second + o.MaxDelay = s.config.WaiterMaxDelay }) - err := waiter.Wait(ctx, &ecs.DescribeServicesInput{...}, 10*time.Minute) + err := waiter.Wait(ctx, &ecs.DescribeServicesInput{...}, s.config.WaiterTimeout)cmd/reduceclustercapacity.go (1)
Line range hint
179-189: Consider validating PutTargets responseAWS EventBridge PutTargets can partially succeed. Consider checking the FailedEntryCount in the response.
- _, err = eventsSvc.PutTargets(ctx, &eventbridge.PutTargetsInput{ + resp, err := eventsSvc.PutTargets(ctx, &eventbridge.PutTargetsInput{ Rule: aws.String(ruleName), Targets: []eventbridgetypes.Target{ { Id: aws.String(targetID), Arn: aws.String(queueArn), }, }, }) - if err != nil { + if err != nil || len(resp.FailedEntries) > 0 { + if len(resp.FailedEntries) > 0 { + err = fmt.Errorf("failed to put targets: %v", resp.FailedEntries) + } return xerrors.Errorf("failed to put a target for interruption warnings: %w", err) }internal/capacity/drainer.go (1)
Line range hint
73-77: Enhance error message clarityConsider including the list of failed instance IDs in the error message to help with debugging.
- return xerrors.Errorf("%d instances should be drained but only %d instances was drained", len(instanceIDs), processedCount) + unprocessedIDs := make([]string, 0) + for _, id := range instanceIDs { + found := false + for _, instance := range instances { + if *instance.Ec2InstanceId == id { + found = true + break + } + } + if !found { + unprocessedIDs = append(unprocessedIDs, id) + } + } + return xerrors.Errorf("%d instances should be drained but only %d instances were drained. Unprocessed instances: %v", + len(instanceIDs), processedCount, unprocessedIDs)internal/capacity/spotfleetrequest_test.go (1)
Line range hint
151-404: Consider using table-driven subtests for exception casesThe exception test cases could be merged with the main test table to create a more unified test structure. This would reduce code duplication and make the test maintenance easier.
func TestSpotFleetRequest_ReduceCapacity(t *testing.T) { tests := []struct { name string drainedInstanceCount int finalTargetCapacity int32 amount int32 config *ec2types.SpotFleetRequestConfigData wantErr bool }{ // existing test cases... { name: "with mixed weighted capacities and a launch template", amount: 4, wantErr: true, config: &ec2types.SpotFleetRequestConfigData{ // existing config... }, }, // other exception test cases... } // single test implementation handling both success and error cases }README.md (2)
15-15: Update product name capitalizationThe term "Apple Silicon" should be capitalized as it's Apple's official product branding.
-For example, you can install the latest binary on a Mac with Apple silicon by the following commands: +For example, you can install the latest binary on a Mac with Apple Silicon by the following commands:🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Did you mean the proper noun “Apple Silicon”?
Context: ...install the latest binary on a Mac with Apple silicon by the following commands: ``` curl -L...(APPLE_PRODUCTS)
17-20: Add language specification to code blockFor better readability and proper syntax highlighting, specify the language for the code block.
-``` +```shell curl -LO https://github.com/abicky/ecsmec/releases/latest/download/ecsmec_darwin_arm64.tar.gz tar xvf ecsmec_darwin_arm64.tar.gz mv ecsmec_darwin_arm64/ecsmec /usr/local/bin/🧰 Tools
🪛 Markdownlint
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
internal/capacity/autoscalinggroup_test.go (1)
Line range hint
389-391: Consider adding error case test scenariosThe test suite thoroughly covers successful scenarios but could benefit from additional test cases for error conditions, such as:
- AWS API errors
- Context cancellation
- Partial failures during instance replacement
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
- .github/workflows/main.yml (2 hunks)
- .gitignore (1 hunks)
- .goreleaser.yml (3 hunks)
- Makefile (2 hunks)
- README.md (1 hunks)
- cmd/recreateservice.go (2 hunks)
- cmd/reduceclustercapacity.go (5 hunks)
- cmd/replaceautoscalinggroupinstances.go (2 hunks)
- cmd/root.go (4 hunks)
- cmd/terminatespotfleetinstances.go (2 hunks)
- go.mod (1 hunks)
- internal/capacity/autoscalinggroup.go (15 hunks)
- internal/capacity/autoscalinggroup_test.go (24 hunks)
- internal/capacity/capacity_test.go (1 hunks)
- internal/capacity/drainer.go (9 hunks)
- internal/capacity/drainer_test.go (7 hunks)
- internal/capacity/interface.go (1 hunks)
- internal/capacity/poller.go (2 hunks)
- internal/capacity/spotfleetrequest.go (8 hunks)
- internal/capacity/spotfleetrequest_test.go (10 hunks)
- internal/service/definition.go (2 hunks)
- internal/service/definition_test.go (1 hunks)
- internal/service/interface.go (1 hunks)
- internal/service/service.go (5 hunks)
- internal/service/service_test.go (8 hunks)
- internal/sliceutil/sliceutil.go (0 hunks)
- internal/sliceutil/sliceutil_test.go (0 hunks)
- internal/testing/capacitymock/generate.go (1 hunks)
- internal/testing/mocks/capacity.go (0 hunks)
- internal/testing/mocks/generate.go (0 hunks)
- internal/testing/servicemock/generate.go (1 hunks)
- internal/testing/testutil/testutil.go (1 hunks)
💤 Files with no reviewable changes (4)
- internal/sliceutil/sliceutil.go
- internal/sliceutil/sliceutil_test.go
- internal/testing/mocks/capacity.go
- internal/testing/mocks/generate.go
🚧 Files skipped from review as they are similar to previous changes (11)
- .gitignore
- .goreleaser.yml
- Makefile
- cmd/recreateservice.go
- cmd/terminatespotfleetinstances.go
- internal/capacity/capacity_test.go
- internal/service/definition.go
- internal/service/interface.go
- internal/testing/capacitymock/generate.go
- internal/testing/servicemock/generate.go
- internal/testing/testutil/testutil.go
🧰 Additional context used
📓 Learnings (5)
.github/workflows/main.yml (1)
Learnt from: abicky PR: abicky/ecsmec#1 File: .github/workflows/main.yml:19-22 Timestamp: 2024-10-28T06:53:47.161Z Learning: When using `setup-go@v4` or later in GitHub Actions workflows, additional caching steps for Go modules are unnecessary because caching is enabled by default.cmd/reduceclustercapacity.go (2)
Learnt from: abicky PR: abicky/ecsmec#1 File: cmd/reduceclustercapacity.go:42-42 Timestamp: 2024-10-28T07:20:01.116Z Learning: In Cobra, to enforce that exactly one of multiple flags is required, you can use `cmd.MarkFlagsOneRequired` and `cmd.MarkFlagsMutuallyExclusive` together.Learnt from: abicky PR: abicky/ecsmec#1 File: cmd/reduceclustercapacity.go:42-42 Timestamp: 2024-10-28T06:59:11.489Z Learning: The method cmd.MarkFlagsRequiredExactlyOne does not exist in the Cobra package.go.mod (1)
Learnt from: abicky PR: abicky/ecsmec#1 File: go.mod:3-3 Timestamp: 2024-10-28T07:02:21.941Z Learning: Go 1.23.2 was released on October 1st, 2024, and it is valid to specify `go 1.23.2` in the `go.mod` file.internal/capacity/autoscalinggroup.go (1)
Learnt from: abicky PR: abicky/ecsmec#1 File: internal/capacity/autoscalinggroup.go:161-161 Timestamp: 2024-10-28T07:07:19.176Z Learning: In Go code, avoid adding redundant parentheses around expressions where operator precedence is clear, as `gofmt` handles formatting and operator precedence.internal/capacity/drainer.go (1)
Learnt from: abicky PR: abicky/ecsmec#1 File: internal/capacity/drainer.go:158-158 Timestamp: 2024-10-28T07:11:42.700Z Learning: Checking `strings.HasPrefix(*t.Group, "service:")` ensures that `t.Group` is not `nil` and has a length of at least 8.
🪛 LanguageTool
README.md
[grammar] ~15-~15: Did you mean the proper noun “Apple Silicon”?
Context: ...install the latest binary on a Mac with Apple silicon by the following commands: ``` curl -L...(APPLE_PRODUCTS)
🪛 Markdownlint
README.md
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🔇 Additional comments (48)
.github/workflows/main.yml (3)
18-22: LGTM! Action versions and Go setup are well configured.The upgrades to the latest stable versions of actions and the use of
go-version-fileinstead of hardcoded version are good practices. Note that Go module caching is automatically handled by setup-go@v5.
37-49: LGTM! Release configuration is up to date.The upgrades to the latest stable versions and the use of the new
--cleanflag in GoReleaser v6 are appropriate changes.#!/bin/bash # Description: Verify GoReleaser configuration is compatible with v6 # Test: Check if .goreleaser.yml exists and is properly configured echo "Checking GoReleaser configuration..." if [ -f ".goreleaser.yml" ] || [ -f ".goreleaser.yaml" ]; then rg -A 5 "dist:" .goreleaser.y* else echo "Warning: GoReleaser configuration file not found" fi
26-26: Verify the Makefile's vet target configuration.The change to use
make vetis good for consistency, but let's verify the target is properly configured.✅ Verification successful
make vettarget is properly configuredThe Makefile contains a properly configured
vettarget that:
- Depends on
$(MOCKS)target to ensure generated code is up to date- Executes
go vet ./...to check all packages in the module🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and configuration of the vet target in Makefile # Test: Check if Makefile exists and contains the vet target echo "Checking Makefile configuration..." if [ -f "Makefile" ]; then echo "Makefile content:" cat Makefile else echo "Error: Makefile not found" exit 1 fiLength of output: 143
Script:
#!/bin/bash # Let's try different approaches to locate and verify the Makefile # 1. Use fd to find any Makefile (case-insensitive) echo "Searching for Makefiles:" fd -i makefile # 2. Use rg to search for 'vet' target in any Makefile echo -e "\nSearching for 'vet' target in Makefiles:" rg -i "vet.*:" -A 5 # 3. Look for any go vet related commands echo -e "\nSearching for go vet commands:" rg "go vet"Length of output: 516
cmd/root.go (2)
44-46: LGTM! Good error handling pattern.The switch to
errors.Asfrom the standard library is appropriate, and the error handling differentiates between runtime and usage errors effectively.
8-9: Verify complete migration to AWS SDK v2.Let's ensure all AWS SDK v1 imports have been replaced across the codebase.
✅ Verification successful
AWS SDK v2 migration is complete
The verification shows that:
- No AWS SDK v1 imports (
github.com/aws/aws-sdk-go/aws) were found in the codebase- AWS SDK v2 imports (
github.com/aws/aws-sdk-go-v2/aws) are consistently used across all AWS-related files🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining AWS SDK v1 imports # Test: Search for aws-sdk-go v1 imports. Expect: No matches echo "Checking for remaining AWS SDK v1 imports..." rg "github.com/aws/aws-sdk-go/aws" echo "Verifying AWS SDK v2 imports are in place..." rg "github.com/aws/aws-sdk-go-v2/aws"Length of output: 1231
internal/capacity/poller.go (5)
7-9: LGTM! Clean import updates for AWS SDK v2The imports are correctly updated to use AWS SDK v2 packages, and the type alias for SQS types improves readability.
32-42: LGTM! Clean context handling implementationThe Poll method correctly implements:
- Context cancellation via select
- Proper error logging
- Continuous polling with context awareness
Line range hint
65-73: LGTM! Proper error handling and batch deletionThe implementation correctly:
- Uses context in AWS API calls
- Handles empty entry lists
- Provides detailed error messages with xerrors
22-22: Verify the SQSAPI interface definitionThe code references
SQSAPItype but its definition is not visible in this file. We need to ensure it properly defines the required AWS SDK v2 methods.Let's locate the interface definition:
Also applies to: 25-30
✅ Verification successful
SQSAPI interface properly defines required AWS SDK v2 methods
The interface definition in
internal/capacity/interface.gocorrectly specifies the required AWS SDK v2 methods:
DeleteMessageBatchwith v2 SDK types and context supportReceiveMessagewith v2 SDK types and context support🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SQSAPI interface definition ast-grep --pattern 'type SQSAPI interface { $$$ }'Length of output: 462
16-17: LGTM! Interface updates align with best practicesThe changes improve the interface by:
- Adding context support for better cancellation control
- Using appropriate AWS SDK v2 types
- Using more precise integer type for wait time
Let's verify that all implementations of this interface have been updated:
✅ Verification successful
Interface implementation is correctly updated
The verification shows that
SQSQueuePolleris the only implementation of thePollerinterface, and its methods have been properly updated to match the interface changes:
- Uses
context.Contextparameter- Uses
sqstypes.Messagetype- Uses
int32forwaitTimeSecondsNo other implementations were found in the codebase that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any struct implementing the Poller interface ast-grep --pattern 'type $_ struct { $$$ } func ($_ *$_) Poll($$$) { $$$ } func ($_ *$_) PollOnce($$$) error { $$$ }'Length of output: 127
Script:
#!/bin/bash # Let's try a different approach to find implementations # First, find all Go files that might contain Poll and PollOnce methods rg -l "Poll.*func.*context\.Context.*sqstypes\.Message" --type go # Then search for struct types and their Poll/PollOnce implementations with context rg "type.*struct|func.*Poll.*context\.Context.*sqstypes\.Message" --type go -A 5Length of output: 3545
cmd/replaceautoscalinggroupinstances.go (3)
4-6: LGTM: AWS SDK v2 imports are correctly updated.The import statements have been properly updated to use the AWS SDK v2 packages, which aligns with the PR's upgrade objectives.
Line range hint
16-33: Verify if command documentation needs updates.The command's documentation should be reviewed to ensure it reflects any behavioral changes introduced by the AWS SDK v2 upgrade, particularly around error handling and timeout behaviors.
#!/bin/bash # Description: Check for command documentation and examples # Check for README or documentation files echo "Checking for command documentation:" fd -t f "README|\.md$" # Check for example usage echo -e "\nChecking for example usage:" rg -l "replace-auto-scaling-group-instances.*example"
31-31: Verify the impact of changing batch-size from Int64 to Int32.The flag type change from Int64 to Int32 needs verification against AWS ECS ListContainerInstances API limits to ensure it won't impact large-scale operations.
✅ Verification successful
Based on the results, I can generate the final response as we have found clear evidence about the constant value and its usage:
Changing batch-size from Int64 to Int32 is safe and appropriate
The
MaxListableContainerInstancesconstant is defined as 100 ininternal/const/ecsconst/const.go, which is well within Int32 bounds. This aligns with AWS ECS ListContainerInstances API limits, and the codebase enforces this limit through validation ininternal/capacity/drainer.go. The type change from Int64 to Int32 is actually more precise and won't impact operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if MaxListableContainerInstances constant is within Int32 bounds # and verify its usage in the codebase # Check the constant definition echo "Checking MaxListableContainerInstances constant:" rg -A 1 "MaxListableContainerInstances.*=.*" # Check other usages of the constant echo -e "\nChecking usages of MaxListableContainerInstances:" rg "MaxListableContainerInstances"Length of output: 1228
internal/service/definition_test.go (3)
7-8: LGTM: Imports updated correctly for AWS SDK v2The imports have been properly updated to use the new AWS SDK v2 packages, with a good practice of using a type alias for ECS types.
Line range hint
77-83: LGTM: Test execution logic remains robustThe test execution logic using
reflect.DeepEqualis appropriate for comparing the structs and remains unchanged, which is good as it will properly handle the new SDK v2 types.
16-16: Verify test coverage for new fieldsThe test struct has been properly updated to use AWS SDK v2 types. However, let's verify that all test cases adequately cover the variations of the new
DesiredCountfield.Also applies to: 21-24
✅ Verification successful
Let me check the test coverage more thoroughly by examining the test cases and their context.
Let me check the test cases in definition_test.go to get a complete picture of the test coverage.
Let me get the complete test cases to see all variations.
Test coverage for DesiredCount is sufficient
The test coverage for the
DesiredCountfield is adequate:
- In
service_test.go, there are test cases covering both non-zero values (viadesiredCountvariable) and explicit zero value checks- In
definition_test.go, all test cases consistently verify the proper conversion ofDesiredCountfromecstypes.Servicetoservice.Definitionwith proper AWS SDK v2 type handling🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are test cases with different DesiredCount values # to ensure proper coverage of edge cases # Search for DesiredCount assignments in test files rg "DesiredCount.*[^1]" --type goLength of output: 1253
Script:
#!/bin/bash # Get more context around the DesiredCount test cases rg "DesiredCount" internal/service/service_test.go -B 5 -A 5Length of output: 1718
Script:
#!/bin/bash # Get context around test cases in definition_test.go rg "func Test.*Definition" internal/service/definition_test.go -A 20Length of output: 732
Script:
#!/bin/bash # Get the full content of definition_test.go to see all test cases cat internal/service/definition_test.goLength of output: 2834
internal/capacity/interface.go (3)
1-10: LGTM! Clean package structure and imports.The package organization and imports are well-structured, using the appropriate AWS SDK v2 packages.
38-41: LGTM! Verify batch operation limits.The interface is minimal and focused. Ensure that batch operations respect AWS SQS limits (max 10 messages per batch).
#!/bin/bash # Search for DeleteMessageBatch usage to verify batch size limits rg -A 5 "DeleteMessageBatch"
20-26: LGTM! Comprehensive EC2 operations coverage.The interface provides essential EC2 operations. Let's verify if all required EC2 operations are covered for the project's needs.
✅ Verification successful
Let me try a different search pattern to find EC2 API usage.
EC2API interface methods are correctly implemented and used
Based on the code analysis, all EC2API interface methods are actively used in the codebase:
DescribeInstances: Used in autoscaling group operationsDescribeSpotFleetInstancesandDescribeSpotFleetRequests: Used in spot fleet managementModifySpotFleetRequest: Used to update spot fleet capacityTerminateInstances: Used in both autoscaling and spot fleet instance terminationThe interface provides exactly the EC2 operations needed for the project's capacity management functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EC2 API calls in the codebase to ensure all required operations are covered rg -t go "ec2\." --no-filename | sort -uLength of output: 6502
Script:
#!/bin/bash # Search for EC2 API method calls in the codebase rg -t go "ec2\.(Describe|Modify|Terminate)" -A 2Length of output: 10529
internal/capacity/spotfleetrequest.go (3)
10-13: LGTM: Clean AWS SDK v2 import migration.The imports have been correctly updated to use AWS SDK Go v2 packages, with good use of type aliases for improved readability.
26-31: Pass context.Context as parameter to NewSpotFleetRequest.This comment was previously made and is still applicable. The function creates a background context internally, which limits the caller's ability to control cancellation and timeouts.
117-119: Verify the drainedCount comparison logic.The comparison
drainedCount.Load() > amount-capacityPerInstancemight allow early exit if capacityPerInstance is greater than 1. Consider if this is the intended behavior.✅ Verification successful
The drainedCount comparison logic is correct and verified
The comparison
drainedCount.Load() > amount-capacityPerInstanceis intentional and correct based on the following evidence:
- The test cases in
TestSpotFleetRequest_ReduceCapacityverify various scenarios with different weighted capacities (1 and 2) and confirm the behavior works as expected.- The logic accounts for weighted capacity correctly:
- When
capacityPerInstance > 1, it means each instance provides multiple units of capacity- Breaking when
drainedCount > amount-capacityPerInstanceensures we don't wait unnecessarily after enough capacity has been drained- This is particularly important for weighted capacity scenarios where draining one instance might satisfy more capacity than needed
The test cases specifically validate this behavior with both regular (capacity=1) and weighted (capacity=2) instances, showing the comparison works correctly in all scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that verify this behavior rg -l 'TestReduceCapacity.*amount.*capacityPerInstance' --type goLength of output: 67
Script:
#!/bin/bash # Let's first look at the full implementation of ReduceCapacity ast-grep --pattern 'func (s $_) ReduceCapacity($$$) { $$$ }' # Also search for any test files related to capacity reduction rg -l "ReduceCapacity" --type goLength of output: 288
Script:
#!/bin/bash # Let's examine the test file content to understand the test cases cat internal/capacity/spotfleetrequest_test.go # Also look at the full implementation to understand the context better cat internal/capacity/spotfleetrequest.goLength of output: 18159
internal/service/service.go (5)
28-54: LGTM! Well-structured context propagationThe context parameter is properly propagated through the service recreation flow while maintaining clear error messages.
Line range hint
56-87: LGTM! Clean AWS SDK v2 migrationThe method has been properly updated to use AWS SDK v2 types and methods while maintaining robust error handling.
Line range hint
122-161: LGTM! Efficient task management implementationThe method effectively uses AWS SDK v2 pagination and properly handles task batches while respecting AWS limits.
Line range hint
164-173: LGTM! Clean and concise implementationThe method has been properly updated to use AWS SDK v2 with context support.
19-19: Verify ECSAPI interface definitionThe
ECSAPItype is used but not defined in this file. Ensure it properly defines all required AWS SDK v2 ECS service methods.✅ Verification successful
ECSAPI interface is properly defined with AWS SDK v2 methods
The ECSAPI interface in
internal/service/interface.gocorrectly defines all the required AWS SDK v2 ECS service methods with proper context and input/output types:
- CreateService
- DeleteService
- DescribeServices
- DescribeTasks
- ListTasks
- UpdateService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ECSAPI interface definition rg -p "type ECSAPI interface" -A 10Length of output: 1807
cmd/reduceclustercapacity.go (2)
4-4: LGTM: AWS SDK v2 imports and context support added correctlyThe import changes appropriately reflect the migration to AWS SDK v2 and the addition of context support.
Also applies to: 8-15
47-47: Verify the impact of changing amount flag type from Int64 to Int32The flag type change from Int64 to Int32 reduces the maximum possible value. Ensure this doesn't impact any use cases requiring large capacity values.
✅ Verification successful
Let's also check the documentation and tests for any hints about expected capacity values:
And let's look at the implementation where this amount is used:
Based on the gathered information, I can now provide the final response:
The Int64 to Int32 type change for amount flag is safe
The change from Int64 to Int32 is appropriate because:
- The amount is only used to reduce capacity, with a validation check that it must be > 0
- The implementation in
SpotFleetRequest.ReduceCapacityandAutoScalingGroup.ReduceCapacityalready uses Int32- The weighted capacity calculations in the codebase also use Int32
- No evidence of large capacity values (> 2^31) being used in the codebase
- The amount is capped by the target capacity (if amount > target capacity, it's reduced to target capacity)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing usage of large capacity values rg -i "amount.*[0-9]{8,}" # Look for numbers with 8+ digits that might exceed Int32Length of output: 27
Script:
#!/bin/bash # Let's search for any usage patterns of the amount flag and capacity values rg -A 5 "amount.*:=" # Look for amount assignments rg -A 5 "capacity.*:=" # Look for capacity assignments rg -A 5 "GetInt32.*amount" # Look for amount flag usage rg -A 5 "GetInt64.*amount" # Look for any remaining Int64 usageLength of output: 1157
Script:
#!/bin/bash # Search in docs and tests rg -i "capacity|amount" "**/*_test.go" rg -i "capacity|amount" "**/*.md"Length of output: 177
Script:
#!/bin/bash # Find where amount is used in the reduce capacity logic ast-grep --pattern 'func $_($$) { $$$ amount := $$$ $$$ }'Length of output: 66
Script:
#!/bin/bash # Let's check how the amount value is used in the actual implementation rg -B 5 -A 10 "amount.*:=" cmd/reduceclustercapacity.go # Let's also check the capacity-related code to understand the scale of values rg -B 5 -A 10 "type.*Capacity" rg -B 5 -A 10 "func.*Capacity"Length of output: 8363
Script:
#!/bin/bash # Let's check tests for capacity values fd -e go -x rg -l "TestReduce" {} fd -e go -x rg -l "capacity.*test" {}Length of output: 240
internal/service/service_test.go (3)
4-17: LGTM: Import and setup changes are correctThe changes appropriately:
- Add context support
- Update to io package (replacing ioutil)
- Migrate to AWS SDK Go v2
- Update mock dependencies
Also applies to: 22-22
37-37: Standardize context parameter usage in mock expectationsThere's inconsistent usage of context parameters across mock expectations. Some use explicit
ctxwhile others usegomock.Any(). For consistency and better testing practices, consider using explicitctxin all mock expectations.Example locations:
- Line 37: Uses explicit
ctx- Line 70: Uses
gomock.Any()- Line 102-103: Uses explicit
ctx- Line 122: Uses
gomock.Any()- Line 135: Uses explicit
ctxAlso applies to: 70-70, 102-103, 122-122, 135-135
173-174: LGTM: Test cases properly updated for AWS SDK v2The test cases have been correctly updated to:
- Use context.Background()
- Handle AWS SDK v2 types
- Maintain comprehensive test coverage including error cases
Also applies to: 186-186, 196-197, 208-208, 235-236, 243-243
internal/capacity/drainer.go (2)
4-29: LGTM: Well-structured SDK v2 migrationThe changes appropriately integrate context support and AWS SDK v2 types. The interface updates follow AWS SDK v2 best practices for better context and resource management.
136-142: Review pagination loop break conditionThe break statement inside the pagination loop might prematurely exit if a single page returns no tasks. Consider moving the empty check outside the pagination loop to ensure all pages are processed.
internal/capacity/spotfleetrequest_test.go (3)
8-16: LGTM: Import statements correctly updated for AWS SDK v2The import statements have been properly updated to use AWS SDK v2 packages and follow the new convention of separate type packages.
39-103: LGTM: AWS SDK v2 migration changes look correctThe test has been properly updated to use AWS SDK v2:
- Context parameter added to all API calls
- Types updated to use ec2types package
- Mock expectations correctly modified for v2 API
155-158: LGTM: Type updates for AWS SDK v2 are correctThe test struct and configuration have been properly updated:
- Capacity values changed from int64 to int32
- SpotFleetRequestConfigData updated to use ec2types package
Also applies to: 165-169
internal/capacity/autoscalinggroup.go (5)
4-16: LGTM: Clean migration to AWS SDK v2The import changes and struct definition updates correctly implement the AWS SDK v2 migration, including proper type adjustments and struct embedding.
Also applies to: 23-31
34-36: Existing comment about context propagation is still valid
184-187: LGTM: AWS SDK v2 API calls properly implementedThe migration to AWS SDK v2 API calls is well-executed, including:
- Proper use of the new input types
- Correct implementation of the v2 waiter pattern
- Appropriate context propagation
Also applies to: 215-229, 235-240
215-215: LGTM: Modern slice handlingGood use of the standard library's
slices.Chunkfunction instead of a custom implementation.
Line range hint
347-383: Verify sorting behavior for edge casesThe instance sorting logic is complex and handles multiple factors:
- Launch time-based sorting
- Availability zone distribution
- Old instance count consideration
Please verify this behavior handles edge cases correctly:
✅ Verification successful
The sorting logic is well-implemented and handles edge cases appropriately
The sorting implementation in
fetchSortedInstanceIDsis correct and handles edge cases through a multi-step approach:
- First sorts instances by launch time (oldest first) using
sort.SliceStable- Groups instances by availability zones while tracking old instance counts
- Sorts availability zones by:
- Instance count (descending) as primary criteria
- Number of old instances (descending) as secondary criteria when instance counts are equal
- Selects instances round-robin from each AZ until reaching the desired count
This approach ensures:
- Even distribution across AZs
- Prioritizes AZs with more instances
- Prefers removing old instances
- Maintains stability through
SliceStableThe implementation is the only instance sorting logic in the codebase and is used consistently for instance termination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the sorting logic implementation across the codebase # Test: Look for similar sorting patterns that might need to be updated # Expected: No inconsistent sorting implementations # Search for availability zone-based sorting patterns rg -A 5 "sort.*AvailabilityZone" # Search for launch time-based sorting rg -A 5 "sort.*LaunchTime" # Look for other instance sorting implementations ast-grep --pattern 'sort.SliceStable($_, func($_, $_) bool { $$$ })'Length of output: 136
Script:
#!/bin/bash # Let's try a different approach to verify the sorting logic # Look for all sort.SliceStable usages with context rg -A 10 "sort\.SliceStable" # Find all functions that deal with instance sorting rg -A 10 "func.*Instance.*sort|sort.*Instance" # Look for any other functions in the same file that might be related fd "autoscalinggroup.go" --exec cat {}Length of output: 18843
internal/capacity/autoscalinggroup_test.go (2)
Line range hint
1-824: Overall implementation looks good!The migration to AWS SDK v2 has been implemented correctly, with proper type updates and context handling throughout most of the file. The test structure is well-organized with comprehensive coverage of successful scenarios.
166-166:⚠️ Potential issueUse
ctxinstead ofcontext.Background()The call to
drainerMock.EXPECT().Drainusescontext.Background()instead of thectxvariable passed to the function. This is inconsistent with the context propagation pattern used throughout the codebase.Apply this diff to fix the issue:
-drainerMock.EXPECT().Drain(context.Background(), gomock.Len(len(instancesToTerminate))), +drainerMock.EXPECT().Drain(ctx, gomock.Len(len(instancesToTerminate))),Likely invalid or redundant comment.
go.mod (3)
3-3: Go version update is validThe update to
go 1.23.2is appropriate and reflects the latest Go release.
7-13: Verify all references to AWS SDK v1 are replacedWith the migration to AWS SDK v2, ensure that all code references to AWS SDK v1 (
github.com/aws/aws-sdk-go) have been updated accordingly.Run the following script to check for any remaining references to AWS SDK v1:
#!/bin/bash # Description: Find any import statements referencing AWS SDK v1. # Expect: No matches found. rg 'import.*"github.com/aws/aws-sdk-go(/|")' --go
6-6: Ensure all imports ofgithub.com/imdario/mergoare updatedThe dependency has been changed to
dario.cat/mergo. Please verify that all import statements have been updated to reflect this change.Run the following script to check for any remaining references to the old
mergopackage:✅ Verification successful
All imports have been properly updated to use
dario.cat/mergoThe verification shows that:
- No instances of the old import
github.com/imdario/mergowere found- The new import
dario.cat/mergois correctly used ininternal/service/definition.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining imports of `github.com/imdario/mergo` # Expect: No matches should be found. rg 'import.*"github.com/imdario/mergo"' --goLength of output: 574
Script:
#!/bin/bash # Search for any remaining imports of the old mergo package rg -t go 'github\.com/imdario/mergo' # Also search for imports of the new package to verify the migration rg -t go 'dario\.cat/mergo'Length of output: 118
internal/capacity/drainer_test.go (1)
Line range hint
1-311: LGTM!The migration to AWS SDK for Go v2 is well-implemented. The test cases have been appropriately updated with context management, and mock expectations reflect the new SDK interfaces.
| ${{ runner.os }}-go- | ||
| go-version-file: go.mod | ||
|
|
||
| - run: go install go.uber.org/mock/mockgen@latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning mockgen to a specific version.
Using @latest could lead to unexpected behavior if breaking changes are released. Consider pinning to a specific version for reproducible builds.
- - run: go install go.uber.org/mock/mockgen@latest
+ - run: go install go.uber.org/mock/mockgen@v0.5.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - run: go install go.uber.org/mock/mockgen@latest | |
| - run: go install go.uber.org/mock/mockgen@v0.5.0 |
| func newConfig(ctx context.Context) (aws.Config, error) { | ||
| region, _ := rootCmd.Flags().GetString("region") | ||
| profile, _ := rootCmd.Flags().GetString("profile") | ||
| return session.NewSessionWithOptions(session.Options{ | ||
| Config: aws.Config{ | ||
| Region: aws.String(region), | ||
| }, | ||
| Profile: profile, | ||
| }) | ||
| return config.LoadDefaultConfig(ctx, config.WithRegion(region), config.WithSharedConfigProfile(profile)) |
There was a problem hiding this comment.
Handle potential errors from flag parsing.
The function ignores errors from GetString calls which could lead to silent failures if flags are malformed.
func newConfig(ctx context.Context) (aws.Config, error) {
- region, _ := rootCmd.Flags().GetString("region")
- profile, _ := rootCmd.Flags().GetString("profile")
+ region, err := rootCmd.Flags().GetString("region")
+ if err != nil {
+ return aws.Config{}, fmt.Errorf("failed to get region flag: %w", err)
+ }
+ profile, err := rootCmd.Flags().GetString("profile")
+ if err != nil {
+ return aws.Config{}, fmt.Errorf("failed to get profile flag: %w", err)
+ }
return config.LoadDefaultConfig(ctx, config.WithRegion(region), config.WithSharedConfigProfile(profile))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func newConfig(ctx context.Context) (aws.Config, error) { | |
| region, _ := rootCmd.Flags().GetString("region") | |
| profile, _ := rootCmd.Flags().GetString("profile") | |
| return session.NewSessionWithOptions(session.Options{ | |
| Config: aws.Config{ | |
| Region: aws.String(region), | |
| }, | |
| Profile: profile, | |
| }) | |
| return config.LoadDefaultConfig(ctx, config.WithRegion(region), config.WithSharedConfigProfile(profile)) | |
| func newConfig(ctx context.Context) (aws.Config, error) { | |
| region, err := rootCmd.Flags().GetString("region") | |
| if err != nil { | |
| return aws.Config{}, fmt.Errorf("failed to get region flag: %w", err) | |
| } | |
| profile, err := rootCmd.Flags().GetString("profile") | |
| if err != nil { | |
| return aws.Config{}, fmt.Errorf("failed to get profile flag: %w", err) | |
| } | |
| return config.LoadDefaultConfig(ctx, config.WithRegion(region), config.WithSharedConfigProfile(profile)) | |
| } |
| func (p *SQSQueuePoller) PollOnce(ctx context.Context, callback func([]sqstypes.Message) ([]sqstypes.DeleteMessageBatchRequestEntry, error), waitTimeSeconds int32) error { | ||
| resp, err := p.sqsSvc.ReceiveMessage(ctx, &sqs.ReceiveMessageInput{ | ||
| MaxNumberOfMessages: sqsconst.MaxReceivableMessages, | ||
| QueueUrl: aws.String(p.queueURL), | ||
| VisibilityTimeout: aws.Int64(10), | ||
| WaitTimeSeconds: aws.Int64(waitTimeSeconds), | ||
| VisibilityTimeout: 10, | ||
| WaitTimeSeconds: waitTimeSeconds, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the visibility timeout configurable
The visibility timeout is currently hard-coded to 10 seconds. This might be insufficient for messages that take longer to process, potentially leading to message reprocessing if the callback execution exceeds this timeout.
Consider making it configurable:
type SQSQueuePoller struct {
queueURL string
sqsSvc SQSAPI
+ visibilityTimeout int32
}
-func NewSQSQueuePoller(queueURL string, sqsSvc SQSAPI) *SQSQueuePoller {
+func NewSQSQueuePoller(queueURL string, sqsSvc SQSAPI, visibilityTimeout int32) *SQSQueuePoller {
return &SQSQueuePoller{
queueURL: queueURL,
sqsSvc: sqsSvc,
+ visibilityTimeout: visibilityTimeout,
}
}
func (p *SQSQueuePoller) PollOnce(ctx context.Context, callback func([]sqstypes.Message) ([]sqstypes.DeleteMessageBatchRequestEntry, error), waitTimeSeconds int32) error {
resp, err := p.sqsSvc.ReceiveMessage(ctx, &sqs.ReceiveMessageInput{
MaxNumberOfMessages: sqsconst.MaxReceivableMessages,
QueueUrl: aws.String(p.queueURL),
- VisibilityTimeout: 10,
+ VisibilityTimeout: p.visibilityTimeout,
WaitTimeSeconds: waitTimeSeconds,
})Committable suggestion was skipped due to low confidence.
| if err := deleteEventRule(cmd.Context(), eventsSvc, ruleNameForInterruptionWarnings, targetID); err != nil { | ||
| return newRuntimeError("failed to delete the event rule \"%s\": %w", ruleNameForInterruptionWarnings, err) | ||
| } | ||
| if err := deleteSQSQueue(sqsSvc, queueURL); err != nil { | ||
| if err := deleteSQSQueue(cmd.Context(), sqsSvc, queueURL); err != nil { | ||
| return newRuntimeError("failed to delete the SQS queue \"%s\": %w", queueNameForInterruptionWarnings, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider wrapping cleanup errors
The cleanup operations (deleteEventRule and deleteSQSQueue) should continue even if one fails. Consider collecting and wrapping all cleanup errors.
- if err := deleteEventRule(cmd.Context(), eventsSvc, ruleNameForInterruptionWarnings, targetID); err != nil {
- return newRuntimeError("failed to delete the event rule \"%s\": %w", ruleNameForInterruptionWarnings, err)
- }
- if err := deleteSQSQueue(cmd.Context(), sqsSvc, queueURL); err != nil {
- return newRuntimeError("failed to delete the SQS queue \"%s\": %w", queueNameForInterruptionWarnings, err)
- }
+ var errs []error
+ if err := deleteEventRule(cmd.Context(), eventsSvc, ruleNameForInterruptionWarnings, targetID); err != nil {
+ errs = append(errs, fmt.Errorf("failed to delete the event rule \"%s\": %w", ruleNameForInterruptionWarnings, err))
+ }
+ if err := deleteSQSQueue(cmd.Context(), sqsSvc, queueURL); err != nil {
+ errs = append(errs, fmt.Errorf("failed to delete the SQS queue \"%s\": %w", queueNameForInterruptionWarnings, err))
+ }
+ if len(errs) > 0 {
+ return newRuntimeError("cleanup errors: %v", errs)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := deleteEventRule(cmd.Context(), eventsSvc, ruleNameForInterruptionWarnings, targetID); err != nil { | |
| return newRuntimeError("failed to delete the event rule \"%s\": %w", ruleNameForInterruptionWarnings, err) | |
| } | |
| if err := deleteSQSQueue(sqsSvc, queueURL); err != nil { | |
| if err := deleteSQSQueue(cmd.Context(), sqsSvc, queueURL); err != nil { | |
| return newRuntimeError("failed to delete the SQS queue \"%s\": %w", queueNameForInterruptionWarnings, err) | |
| } | |
| var errs []error | |
| if err := deleteEventRule(cmd.Context(), eventsSvc, ruleNameForInterruptionWarnings, targetID); err != nil { | |
| errs = append(errs, fmt.Errorf("failed to delete the event rule \"%s\": %w", ruleNameForInterruptionWarnings, err)) | |
| } | |
| if err := deleteSQSQueue(cmd.Context(), sqsSvc, queueURL); err != nil { | |
| errs = append(errs, fmt.Errorf("failed to delete the SQS queue \"%s\": %w", queueNameForInterruptionWarnings, err)) | |
| } | |
| if len(errs) > 0 { | |
| return newRuntimeError("cleanup errors: %v", errs) | |
| } |
| tasksStoppedWaiter := ecs.NewTasksStoppedWaiter(d.ecsSvc, func(o *ecs.TasksStoppedWaiterOptions) { | ||
| o.MaxDelay = 6 * time.Second | ||
| }) | ||
| for arns := range slices.Chunk(allTaskArns, ecsconst.MaxDescribableTasks) { | ||
| err := tasksStoppedWaiter.Wait(ctx, &ecs.DescribeTasksInput{ | ||
| Cluster: aws.String(d.cluster), | ||
| Tasks: arns, | ||
| }) | ||
| }, 10*time.Minute) | ||
| if err != nil { | ||
| return xerrors.Errorf("failed to wait for the tasks to stop: %w", err) | ||
| return xerrors.Errorf("failed to wait for tasks to stop: %w", err) | ||
| } | ||
| } | ||
|
|
||
| log.Printf("Wait for all the services in the cluster \"%s\" to become stable\n", d.cluster) | ||
| for names := range sliceutil.ChunkSlice(allServiceNames, ecsconst.MaxDescribableServices) { | ||
| err := d.ecsSvc.WaitUntilServicesStable(&ecs.DescribeServicesInput{ | ||
| servicesStableWaiter := ecs.NewServicesStableWaiter(d.ecsSvc, func(o *ecs.ServicesStableWaiterOptions) { | ||
| o.MaxDelay = 15 * time.Second | ||
| }) | ||
| for names := range slices.Chunk(allServiceNames, ecsconst.MaxDescribableServices) { | ||
| err := servicesStableWaiter.Wait(ctx, &ecs.DescribeServicesInput{ | ||
| Cluster: aws.String(d.cluster), | ||
| Services: names, | ||
| }) | ||
| }, 10*time.Minute) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making timeout values configurable
The timeout values (10 minutes) and delay intervals (6 seconds, 15 seconds) are hardcoded. Consider making these configurable through the Drainer interface to allow for different scenarios and requirements.
type drainer struct {
cluster string
batchSize int32
ecsSvc ECSAPI
+ taskTimeout time.Duration
+ serviceTimeout time.Duration
+ taskDelay time.Duration
+ serviceDelay time.Duration
}
func NewDrainer(cluster string, batchSize int32, ecsSvc ECSAPI) (Drainer, error) {
if batchSize > ecsconst.MaxListableContainerInstances {
return nil, xerrors.Errorf("batchSize greater than %d is not supported", ecsconst.MaxListableContainerInstances)
}
return &drainer{
cluster: cluster,
batchSize: batchSize,
ecsSvc: ecsSvc,
+ taskTimeout: 10 * time.Minute,
+ serviceTimeout: 10 * time.Minute,
+ taskDelay: 6 * time.Second,
+ serviceDelay: 15 * time.Second,
}, nil
}Committable suggestion was skipped due to low confidence.
| // For InstanceTerminatedWaiter | ||
| ec2Mock.EXPECT().DescribeInstances(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, input *ec2.DescribeInstancesInput, _ ...func(*ec2.Options)) (*ec2.DescribeInstancesOutput, error) { | ||
| instanceIds := make([]string, len(instances)) | ||
| for i, instance := range instances { | ||
| instanceIds[i] = *instance.InstanceId | ||
| } | ||
|
|
||
| if !testutil.MatchSlice(input.InstanceIds, instanceIds) { | ||
| t.Errorf("input.InstanceIds = %v; want %v", input.InstanceIds, instanceIds) | ||
| } | ||
|
|
||
| instances := make([]ec2types.Instance, len(instanceIds)) | ||
| for i, id := range instanceIds { | ||
| instances[i] = ec2types.Instance{ | ||
| InstanceId: aws.String(id), | ||
| State: &ec2types.InstanceState{ | ||
| Name: "terminated", | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| return &ec2.DescribeInstancesOutput{ | ||
| Reservations: []ec2types.Reservation{ | ||
| { | ||
| Instances: instances, | ||
| }, | ||
| }, | ||
| }, nil | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting instance state setup into a helper function
The instance state setup for the InstanceTerminatedWaiter is quite verbose. Consider extracting it into a helper function to improve test readability.
func createTerminatedInstances(t *testing.T, instanceIds []string) []ec2types.Instance {
instances := make([]ec2types.Instance, len(instanceIds))
for i, id := range instanceIds {
instances[i] = ec2types.Instance{
InstanceId: aws.String(id),
State: &ec2types.InstanceState{
Name: "terminated",
},
}
}
return instances
}
README.md
Outdated
| For example, you can install the latest binary on a Mac with Apple silicon by the following commands: | ||
|
|
||
| ``` | ||
| curl -LO https://github.com/abicky/ecsmec/releases/latest/download/ecsmec_darwin_amd64.tar.gz | ||
| tar xvf ecsmec_darwin_amd64.tar.gz | ||
| mv ecsmec_darwin_amd64/ecsmec /usr/local/bin/ | ||
| curl -LO https://github.com/abicky/ecsmec/releases/latest/download/ecsmec_darwin_arm64.tar.gz | ||
| tar xvf ecsmec_darwin_arm64.tar.gz | ||
| mv ecsmec_darwin_arm64/ecsmec /usr/local/bin/ |
There was a problem hiding this comment.
💡 Codebase verification
Installation instructions should include both Intel and Apple Silicon options
The README currently only shows installation instructions for Apple Silicon (arm64) Macs, while the repository also supports Intel-based Macs through Homebrew and source installation. For clarity and completeness:
- The binary installation section should include both architectures:
# For Apple Silicon (M1/M2) Macs: curl -LO https://github.com/abicky/ecsmec/releases/latest/download/ecsmec_darwin_arm64.tar.gz # For Intel Macs: curl -LO https://github.com/abicky/ecsmec/releases/latest/download/ecsmec_darwin_amd64.tar.gz
Since Homebrew support is already documented as an alternative installation method (which handles architecture selection automatically), users have a reliable fallback option.
🔗 Analysis chain
Verify binary availability for all architectures
While updating the instructions for Apple Silicon is great, we should ensure that Intel Mac users can still find appropriate installation instructions.
Consider adding a note about architecture-specific binaries and how Intel Mac users should download the darwin_amd64 version instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both ARM64 and AMD64 binaries are available in the latest release
gh release download --repo abicky/ecsmec latest --pattern "*darwin_*" --dry-run
Length of output: 679
Script:
#!/bin/bash
# Let's try a different approach to check the release assets
gh release view --repo abicky/ecsmec latest --json assets -q '.assets[].name'
Length of output: 97
Script:
#!/bin/bash
# Let's check the repository's README.md for any other architecture-related information
cat README.md
Length of output: 14917
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Did you mean the proper noun “Apple Silicon”?
Context: ...install the latest binary on a Mac with Apple silicon by the following commands: ``` curl -L...(APPLE_PRODUCTS)
🪛 Markdownlint
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
internal/testing/testutil/testutil.go (1)
12-14: Well-designed context matcher utility!The
AnyContextmatcher is a valuable addition that will simplify testing of context-aware functions, especially important with the AWS SDK v2 upgrade where context usage is more prevalent.Consider documenting common usage patterns of this matcher in the package documentation, as it will be frequently used across tests with the new AWS SDK v2 context-aware methods.
internal/service/service_test.go (1)
Line range hint
213-246: Consider expanding error test casesWhile the exception tests cover 'unknown service' and 'inactive service' scenarios, consider adding tests for:
- API errors from AWS SDK calls
- Context cancellation scenarios
- Rate limiting or throttling responses
Would you like me to help generate additional test cases for these scenarios?
internal/capacity/autoscalinggroup_test.go (2)
Line range hint
432-474: Consider extracting common test setup codeThe test cases contain repeated setup code for creating mocks, contexts, and common test data. Consider extracting this into helper functions to improve maintainability and reduce duplication.
Example refactor:
func setupAutoScalingTest(t *testing.T) (context.Context, *gomock.Controller, *capacitymock.MockAutoScalingAPI, *capacitymock.MockEC2API, *capacitymock.MockDrainer) { ctrl := gomock.NewController(t) ctx := context.Background() asMock := capacitymock.NewMockAutoScalingAPI(ctrl) ec2Mock := capacitymock.NewMockEC2API(ctrl) drainerMock := capacitymock.NewMockDrainer(ctrl) return ctx, ctrl, asMock, ec2Mock, drainerMock }Usage:
func TestAutoScalingGroup_ReplaceInstances(t *testing.T) { ctx, ctrl, asMock, ec2Mock, drainerMock := setupAutoScalingTest(t) defer ctrl.Finish() // Test-specific code... }Also applies to: 484-553, 708-823
4-4: TODO comment can be removedThe TODO comment about adding tests has been addressed with the implementation of comprehensive test cases covering various scenarios including:
- Basic instance replacement
- Uneven distribution across availability zones
- Capacity reduction
- State restoration
Remove the TODO comment as it's no longer applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- internal/capacity/autoscalinggroup_test.go (24 hunks)
- internal/capacity/drainer_test.go (7 hunks)
- internal/capacity/spotfleetrequest_test.go (10 hunks)
- internal/service/service_test.go (8 hunks)
- internal/testing/testutil/testutil.go (1 hunks)
🔇 Additional comments (17)
internal/testing/testutil/testutil.go (1)
4-10: LGTM! Clean upgrade to go.uber.org/mockThe import changes align well with the package upgrade objectives, and the addition of context and reflect imports is properly structured.
internal/service/service_test.go (3)
4-5: LGTM: Successfully migrated imports and setupThe changes correctly:
- Update to AWS SDK v2 imports
- Replace deprecated
ioutil.Discardwithio.Discard- Add necessary type imports for ECS
Also applies to: 11-14, 17-17, 22-22
70-84: Maintain consistent context usage in mock expectationsThe mock expectations still use
testutil.AnyContext()in some places while using explicitctxin others. This inconsistency was noted in previous reviews and remains to be addressed.Let's verify the context usage pattern across the codebase:
#!/bin/bash # Search for context parameter usage patterns in mock expectations rg --type go 'EXPECT\(\).*Context|AnyContext' .Also applies to: 122-132
11-13: Verify AWS SDK v2 migration completenessThe migration to AWS SDK v2 appears complete with proper type updates and context propagation.
Let's verify no AWS SDK v1 remnants exist:
Also applies to: 31-32, 42-42, 76-81
✅ Verification successful
AWS SDK v2 migration is complete
The verification confirms that all AWS SDK imports are using v2 (
aws-sdk-go-v2). The single v1 SDK reference found is in a comment discussing a historical issue. No v1 type conversions, pointers, or legacy session/service imports were detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining AWS SDK v1 imports or types rg --type go "github.com/aws/aws-sdk-go(?!/v2)" . rg --type go "aws.Int64|aws.Int64Value|\*int64" .Length of output: 364
Script:
#!/bin/bash # Check for any remaining AWS SDK v1 imports rg --type go --pcre2 "github\.com/aws/aws-sdk-go(?!/v2)" . # Check for AWS SDK v1 type conversions and pointers rg --type go "aws\.Int64\(" . rg --type go "aws\.Int64Value\(" . rg --type go "\*int64" . # Check for other AWS SDK v1 specific types and methods rg --type go "github\.com/aws/aws-sdk-go/aws/session" . rg --type go "github\.com/aws/aws-sdk-go/service" .Length of output: 5749
internal/capacity/spotfleetrequest_test.go (4)
8-16: LGTM: Import changes align with AWS SDK v2 migrationThe updated imports correctly reflect the transition to AWS SDK v2 and the new mock package structure.
39-103: LGTM: Context handling and type updates are correctThe test correctly implements context propagation and AWS SDK v2 type changes. The mock expectations properly verify the context parameter and type conversions.
Line range hint
151-189: LGTM: Type changes and test structure updatesThe test cases correctly implement the AWS SDK v2 type changes, including the transition from int64 to int32 for capacity values and the updated ec2types package usage.
156-158: Verify int64 to int32 type changes across the codebaseThe change from int64 to int32 for capacity-related values needs to be consistently applied across the codebase.
internal/capacity/autoscalinggroup_test.go (1)
10-16: AWS SDK v2 migration looks goodThe migration to AWS SDK v2 has been properly implemented with:
- Correct import statements
- Updated type references (e.g.,
autoscalingtypes.Instance,ec2types.Instance)- Context propagation in method signatures
internal/capacity/drainer_test.go (8)
4-4: LGTM!The changes to initialize and pass the context to various functions are correct and consistent with the AWS SDK for Go v2 requirements.
Also applies to: 25-26, 128-129, 162-163, 263-264
9-13: LGTM!The import statements have been updated correctly to reflect the new package paths for AWS SDK v2 and the transition to the
capacitymockpackage for mocking AWS services.Also applies to: 16-17
27-32: LGTM!The changes to the initialization and usage of container instance ARNs, container instances, and messages are consistent with the transition to value types in the AWS SDK for Go v2.
Also applies to: 168-170, 187-189, 268-270
43-48: LGTM!The expectations set on the mock ECS API have been updated correctly to accommodate the new method signatures that include context and options parameters. The handling of return types has also been adjusted to match the new SDK's output structures.
Also applies to: 55-64, 67-68, 80-81, 89-99, 101-112, 138-143, 195-200, 203-204, 207-217, 220-221, 233-234, 242-242, 276-281
180-180: LGTM!The initialization of the
expectedEntriesslice is correct and consistent with the changes to thesqstypes.DeleteMessageBatchRequestEntrystructure in the AWS SDK for Go v2.
291-291: Correct error condition in test assertionIn lines 288-290, the error check seems incorrect. You're checking
err != nil, but the message indicates you expecterrto be nil.Apply this diff to correct the condition:
-if err != nil { - t.Errorf("err = nil; want non-nil") +if err == nil { + t.Errorf("err = nil; want non-nil")
311-311: Correct the error check conditionAt lines 308-310, the error check condition is inconsistent with the error message. Since you expect an error to be returned when processing empty messages, the condition should check
if err == nilto correctly detect when an error is not returned.Apply this diff to correct the condition:
-if err != nil { +if err == nil { t.Errorf("err = nil; want non-nil") }
119-119: Verify error handling in the updatedDrainandProcessInterruptionsmethods.Please ensure that the error handling logic remains correct after the changes to the method signatures and the introduction of the context parameter.
Run the following script to verify the error handling:
Also applies to: 151-151, 249-249, 289-289, 309-309
✅ Verification successful
Let me analyze the error handling by checking the implementation of these methods.
Let me try a different approach to find the implementations.
Error handling is properly implemented in both methods
The error handling in both
DrainandProcessInterruptionsmethods is correctly implemented after the context parameter addition:
Drainmethod:
- Properly wraps and returns errors from
processContainerInstances- Returns appropriate error when no instances are found
- Error handling is consistent across all call sites (autoscalinggroup.go, spotfleetrequest.go)
ProcessInterruptionsmethod:
- Handles empty message list gracefully
- Properly handles JSON unmarshaling errors
- Error handling is consistent in the spotfleetrequest.go caller
The context parameter is correctly propagated through the error paths, and all error cases are properly wrapped with descriptive messages using
xerrors.Errorf.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in the `Drain` and `ProcessInterruptions` methods. # Test: Search for the method calls and check the error handling. # Expect: Consistent error handling with the previous implementation. rg -A 5 $'drainer\.Drain\(' rg -A 5 $'drainer\.ProcessInterruptions\('Length of output: 3362
Script:
#!/bin/bash # Search for the implementation of Drain and ProcessInterruptions methods ast-grep --pattern 'func ($_ *Drainer) Drain(ctx context.Context, $_) error { $$$ }' ast-grep --pattern 'func ($_ *Drainer) ProcessInterruptions(ctx context.Context, $_) ($_, error) { $$$ }'Length of output: 198
Script:
#!/bin/bash # Search for the implementation of Drain and ProcessInterruptions methods rg -U "func.*Drain.*context\.Context.*\) error \{" -A 20 rg -U "func.*ProcessInterruptions.*context\.Context.*\) \(.*error\) \{" -A 20Length of output: 3209
| args := make([]any, 0, len(calls)) | ||
| for _, call := range calls { | ||
| args = append(args, call) | ||
| } | ||
| gomock.InOrder(args...) |
There was a problem hiding this comment.
Add input validation to prevent panics
While the implementation is correct, it could panic if calls is empty. Consider adding input validation.
Apply this improvement:
func InOrder(calls ...*gomock.Call) *gomock.Call {
+ if len(calls) == 0 {
+ panic("InOrder requires at least one call")
+ }
args := make([]any, 0, len(calls))
for _, call := range calls {
args = append(args, call)
}
gomock.InOrder(args...)
return calls[len(calls)-1]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args := make([]any, 0, len(calls)) | |
| for _, call := range calls { | |
| args = append(args, call) | |
| } | |
| gomock.InOrder(args...) | |
| if len(calls) == 0 { | |
| panic("InOrder requires at least one call") | |
| } | |
| args := make([]any, 0, len(calls)) | |
| for _, call := range calls { | |
| args = append(args, call) | |
| } | |
| gomock.InOrder(args...) |
| messages := make([]sqstypes.Message, tt.drainedInstanceCount) | ||
| entries := make([]sqstypes.DeleteMessageBatchRequestEntry, tt.drainedInstanceCount) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a helper function for test message setup
The setup of messages and entries arrays could be extracted into a helper function to improve test readability.
func createTestMessages(count int) ([]sqstypes.Message, []sqstypes.DeleteMessageBatchRequestEntry) {
messages := make([]sqstypes.Message, count)
entries := make([]sqstypes.DeleteMessageBatchRequestEntry, count)
return messages, entries
}
abicky
force-pushed
the
upgrade-go-and-packages
branch
from
dc36f71
to
338daee
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
internal/capacity/poller.go (1)
33-43: Consider enhancing error logging
While the error handling is correct, consider adding more context to the log message to help with debugging.
- if err := p.PollOnce(ctx, callback, sqsconst.WaitTimeSecondsForLongPolling); err != nil {
- log.Printf("[WARNING] %+v\n", err)
+ if err := p.PollOnce(ctx, callback, sqsconst.WaitTimeSecondsForLongPolling); err != nil {
+ log.Printf("[WARNING] SQS polling error for queue %s: %+v\n", p.queueURL, err)internal/capacity/spotfleetrequest.go (1)
168-176: Enhance error messages for weighted capacity validation
The error messages for unsupported weighted capacities could be more specific by including the actual values that caused the validation to fail.
-return 0, xerrors.Errorf("currently float weighted capacities are not supported")
+return 0, xerrors.Errorf("float weighted capacity %f is not supported", *o.WeightedCapacity)
-return 0, xerrors.Errorf("currently mixed weighted capacities are not supported")
+return 0, xerrors.Errorf("mixed weighted capacities are not supported: found %d, expected %d", int32(*o.WeightedCapacity), weightedCapacity)Also applies to: 185-193
internal/capacity/drainer.go (1)
142-142: Consider removing unnecessary break statement
With the AWS SDK v2 paginator implementation, this break statement appears unnecessary as the paginator will naturally stop when there are no more pages.
internal/capacity/autoscalinggroup.go (3)
Line range hint 42-75: Consider enhancing error handling in ReplaceInstances
While the context handling is correct, the error messages could be more specific about which operation failed during instance replacement.
Consider updating error messages to include more context:
- return xerrors.Errorf("failed to launch new instances: %w", err)
+ return xerrors.Errorf("failed to launch %d new instances: %w", len(oldInstanceIDs), err)Line range hint 78-118: Enhance error handling in reload method
The error handling could be more specific about which tag parsing failed.
Consider wrapping the tag parsing errors with more context:
- return xerrors.Errorf("ecsmec:OriginalDesiredCapacity is invalid (%s): %w", *t.Value, err)
+ return xerrors.Errorf("failed to parse tag ecsmec:OriginalDesiredCapacity=%q as int32: %w", *t.Value, err)235-242: Consider extracting timeout constant
The instance termination timeout of 10 minutes is hardcoded. Consider extracting it as a constant for better maintainability.
Add a constant at the package level:
+const instanceTerminationTimeout = 10 * time.Minute
+
waiter := ec2.NewInstanceTerminatedWaiter(asg.ec2Svc, func(o *ec2.InstanceTerminatedWaiterOptions) {
o.MaxDelay = 15 * time.Second
})
err = waiter.Wait(ctx, &ec2.DescribeInstancesInput{
InstanceIds: sortedInstanceIDs,
- }, 10*time.Minute)
+ }, instanceTerminationTimeout)README.md (1)
15-15: Use proper capitalization for "Apple Silicon"
The term "Apple Silicon" should be capitalized as it's a proper noun referring to Apple's processor architecture.
-For example, you can install the latest binary on a Mac with Apple silicon by the following commands:
+For example, you can install the latest binary on a Mac with Apple Silicon by the following commands:🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Did you mean the proper noun “Apple Silicon”?
Context: ...install the latest binary on a Mac with Apple silicon by the following commands: ``` curl -L...
(APPLE_PRODUCTS)
🛑 Comments failed to post (5)
internal/capacity/spotfleetrequest.go (1)
86-98: 🛠️ Refactor suggestion
Consider using parent context for polling
The polling goroutine creates a new background context which doesn't inherit the parent context's deadline. This could lead to the goroutine continuing to run even after the parent context is cancelled.
-ctxForPoll, cancel := context.WithCancel(context.Background()) +ctxForPoll, cancel := context.WithCancel(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.drainedCount := &atomic.Int32{} ctxForPoll, cancel := context.WithCancel(ctx) defer cancel() go func() { poller.Poll(ctxForPoll, func(messages []sqstypes.Message) ([]sqstypes.DeleteMessageBatchRequestEntry, error) { entries, err := drainer.ProcessInterruptions(ctx, messages) if err != nil { return nil, xerrors.Errorf("failed to process interruptions: %w", err) } drainedCount.Add(int32(len(entries)) * capacityPerInstance) return entries, nil })internal/service/service.go (2)
79-84: 🛠️ Refactor suggestion
Consider enhancing retry logic with exponential backoff
While the retry logic works, consider implementing exponential backoff for better handling of temporary failures.
Here's a suggested implementation:
-err = retryOnServiceCreationTempErr(func() error { - return s.createAndWaitUntilStable(ctx, config) -}, 60) +err = retry.Do( + func() error { + return s.createAndWaitUntilStable(ctx, config) + }, + retry.Context(ctx), + retry.Attempts(60), + retry.DelayType(retry.BackOffDelay), + retry.RetryIf(func(err error) bool { + var e *ecstypes.InvalidParameterException + return errors.As(err, &e) && + e.ErrorMessage() == "Unable to Start a service that is still Draining." + }), +)Consider adding the following import:
import "github.com/avast/retry-go/v4"
95-101: 🛠️ Refactor suggestion
Consider making timeout values configurable
The waiter delay and timeout values are currently hardcoded. Consider making these configurable to support different deployment scenarios.
Example implementation:
+type ServiceConfig struct { + WaiterMaxDelay time.Duration + WaiterTimeout time.Duration +} + +func NewService(ecsSvc ECSAPI, config *ServiceConfig) *Service { if config == nil { config = &ServiceConfig{ WaiterMaxDelay: 15 * time.Second, WaiterTimeout: 10 * time.Minute, } } return &Service{ ecsSvc: ecsSvc, + config: config, } } func (s *Service) createAndWaitUntilStable(ctx context.Context, config *ecs.CreateServiceInput) error { waiter := ecs.NewServicesStableWaiter(s.ecsSvc, func(o *ecs.ServicesStableWaiterOptions) { - o.MaxDelay = 15 * time.Second + o.MaxDelay = s.config.WaiterMaxDelay }) err := waiter.Wait(ctx, &ecs.DescribeServicesInput{ Cluster: config.Cluster, Services: []string{*config.ServiceName}, - }, 10*time.Minute) + }, s.config.WaiterTimeout)Committable suggestion was skipped due to low confidence.
internal/service/definition.go (2)
50-50:
⚠️ Potential issuePrevent potential index out of range errors when accessing
s.Deployments[0]Accessing
s.Deployments[0]without checking ifs.Deploymentsis non-empty could lead to a runtime panic ifs.Deploymentsis empty. Ensure thats.Deploymentshas at least one element before accessing index 0.Apply this diff to safely access
s.Deployments[0]:return &Definition{ ... + var serviceConnectConfiguration *ecstypes.ServiceConnectConfiguration + var volumeConfigurations []ecstypes.VolumeConfiguration + if len(s.Deployments) > 0 { + serviceConnectConfiguration = s.Deployments[0].ServiceConnectConfiguration + volumeConfigurations = s.Deployments[0].VolumeConfigurations + } + ServiceConnectConfiguration: serviceConnectConfiguration, ... VolumeConfigurations: volumeConfigurations, }Also applies to: 54-54
58-119: 🛠️ Refactor suggestion
Add unit tests for the new
GoStringandprettifymethodsThe newly added
GoStringandprettifymethods use reflection to provide a string representation of theDefinitionstruct. To ensure correctness and prevent potential runtime errors due to reflection usage, it's recommended to add unit tests covering various scenarios.Would you like assistance in creating unit tests for these methods?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
internal/capacity/spotfleetrequest.go (1)
Line range hint 162-191: Consider improving type conversion clarity.
The weighted capacity checks could be more explicit about the requirements. Consider adding constants or documentation to explain why float and mixed weighted capacities are not supported.
func (sfr *SpotFleetRequest) weightedCapacity() (int32, error) {
+ // We only support integer weighted capacities to ensure consistent capacity calculations
weightedCapacity := int32(0)internal/service/service_test.go (2)
Line range hint 27-85: LGTM! Helper function expectCopy has been correctly updated for AWS SDK v2.
The changes appropriately:
- Add context parameter
- Update to SDK v2 types
- Handle mock expectations correctly
For consistency, consider using the explicit ctx parameter instead of testutil.AnyContext() in the DescribeServices mock at line 71.
Line range hint 90-141: LGTM! Helper function expectStopAndDelete has been correctly updated for AWS SDK v2.
The changes appropriately:
- Add context parameter
- Update to SDK v2 types
- Handle mock expectations correctly
For consistency, consider using the explicit ctx parameter instead of testutil.AnyContext() in the DescribeTasks mock at line 123.
internal/capacity/drainer.go (1)
Line range hint 52-75: Consider adding context timeout handling
While the context is properly propagated, consider adding a timeout to prevent indefinite waiting during user input. This is especially important as the function waits for user confirmation.
func (d *drainer) Drain(ctx context.Context, instanceIDs []string) error {
+ // Add timeout for the entire operation
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Minute)
+ defer cancel()
+
processedCount := 0
err := d.processContainerInstances(ctx, instanceIDs, func(instances []ecstypes.ContainerInstance) error {README.md (3)
17-20: Add language specifier to code block
Add bash or shell as the language specifier to the code block for proper syntax highlighting.
-```
+```bash
curl -LO https://github.com/abicky/ecsmec/releases/latest/download/ecsmec_darwin_arm64.tar.gz
tar xvf ecsmec_darwin_arm64.tar.gz
mv ecsmec_darwin_arm64/ecsmec /usr/local/bin/🧰 Tools
🪛 Markdownlint
17-17: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
143-152: Fix JSON indentation
The new IAM permission block has incorrect indentation. It should be at the same level as other permission blocks.
],
- {
- "Effect": "Allow",
- "Action": [
- "iam:PassRole"
- ],
- "Resource": [
- "arn:aws:iam::<account-id>:role/<role_for_volume_configurations>"
- ]
- }
+ },
+ {
+ "Effect": "Allow",
+ "Action": [
+ "iam:PassRole"
+ ],
+ "Resource": [
+ "arn:aws:iam::<account-id>:role/<role_for_volume_configurations>"
+ ]
+ }437-442: Add language specifier to help output code block
Add console as the language specifier to the command help output code block.
-```
+```console
$ ecsmec terminate-spot-fleet-instances --helpThe command rename and description update provide better clarity about the command's purpose and when it can be used.
🧰 Tools
🪛 Markdownlint
439-439: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
- .goreleaser.yml (2 hunks)
- README.md (3 hunks)
- cmd/terminatespotfleetinstances.go (3 hunks)
- go.mod (1 hunks)
- internal/capacity/autoscalinggroup.go (14 hunks)
- internal/capacity/drainer.go (9 hunks)
- internal/capacity/poller.go (2 hunks)
- internal/capacity/spotfleetrequest.go (8 hunks)
- internal/service/definition.go (3 hunks)
- internal/service/definition_test.go (1 hunks)
- internal/service/service.go (5 hunks)
- internal/service/service_test.go (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .goreleaser.yml
- internal/capacity/poller.go
- internal/service/definition_test.go
🧰 Additional context used
📓 Learnings (2)
go.mod (1)
Learnt from: abicky
PR: abicky/ecsmec#1
File: go.mod:3-3
Timestamp: 2024-10-28T07:02:21.941Z
Learning: Go 1.23.2 was released on October 1st, 2024, and it is valid to specify `go 1.23.2` in the `go.mod` file.
internal/capacity/autoscalinggroup.go (2)
Learnt from: abicky
PR: abicky/ecsmec#1
File: internal/capacity/autoscalinggroup.go:161-161
Timestamp: 2024-10-28T07:07:19.176Z
Learning: In Go code, avoid adding redundant parentheses around expressions where operator precedence is clear, as `gofmt` handles formatting and operator precedence.
Learnt from: abicky
PR: abicky/ecsmec#1
File: internal/capacity/autoscalinggroup.go:34-36
Timestamp: 2024-10-29T07:04:02.226Z
Learning: In `internal/capacity/autoscalinggroup.go`, avoid passing `context.Context` to `NewAutoScalingGroup` because the method completes quickly, and passing context might lead users to think it's used in methods of `AutoScalingGroup`.
🪛 LanguageTool
README.md
[grammar] ~15-~15: Did you mean the proper noun “Apple Silicon”?
Context: ...install the latest binary on a Mac with Apple silicon by the following commands: ``` curl -L...
(APPLE_PRODUCTS)
🪛 Markdownlint
README.md
17-17: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
439-439: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (27)
go.mod (2)
7-13: Verify AWS SDK v2 migration impact.
The migration from AWS SDK v1 to v2 introduces significant changes in the API. Please ensure:
- All AWS service clients are properly initialized with context
- Error handling is updated for v2 style errors
- All pagination code is updated to use v2 paginators
Let's verify the migration:
✅ Verification successful
Let me try a different approach to verify the AWS SDK migration:
Let me check one more aspect to verify the AWS SDK v2 migration:
Based on the verification results, I can now provide a final response:
AWS SDK v2 migration appears properly implemented
The codebase shows consistent and proper usage of AWS SDK v2:
- All imports correctly use
github.com/aws/aws-sdk-go-v2and its subpackages - Context is properly used in AWS operations as seen in the test files
- AWS SDK v2 client types are properly imported and used across multiple packages
- No traces of AWS SDK v1 were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining v1 SDK imports
echo "Checking for remaining v1 SDK imports..."
rg "github.com/aws/aws-sdk-go(?!/v2)" -l
# Check for proper context usage with v2 SDK
echo "Checking context usage with v2 SDK..."
ast-grep --pattern 'aws.NewConfig()' || ast-grep --pattern 'aws.Config{}'
Length of output: 518
Script:
#!/bin/bash
# Check for remaining v1 SDK imports with PCRE2
echo "Checking for remaining v1 SDK imports..."
rg --pcre2 "github\.com/aws/aws-sdk-go(?!-v2)" -l
# Check AWS SDK v2 client initialization patterns
echo "Checking AWS SDK v2 client initialization..."
rg "New.*Client\(" -A 2
# Check for context usage in AWS operations
echo "Checking context usage in AWS operations..."
rg "\.(Describe|Create|Delete|Update|Get|List|Put|Send).*Context\(" -A 1
Length of output: 2885
Script:
#!/bin/bash
# Check for AWS SDK v2 imports and client initialization in non-test files
echo "Checking AWS SDK v2 usage in main code..."
rg "github.com/aws/aws-sdk-go-v2" -B 2 -A 2 --type go --glob '!*_test.go'
# Check for AWS SDK v2 error handling patterns
echo "Checking error handling patterns..."
rg "smithy\.APIError" -A 2
Length of output: 5734
37-38: Track AWS SDK issue resolution.
The replace directive is using a fork of the ECS service package. To ensure maintainability:
- Add a watch on AWS SDK issue #2859
- Set up automated checks for when the issue is resolved
- Consider documenting the specific changes in the fork
Let's verify the fork's status:
✅ Verification successful
Temporary replace directive is valid and issue is still open
The verification confirms:
- Issue #2859 in aws/aws-sdk-go-v2 is still open
- The fork at abicky/aws-sdk-go-v2 is in sync with the main repository
Let's verify the specific changes in the fork:
Temporary replace directive is well-documented and necessary
The replace directive is justified and properly managed:
- It fixes a critical bug where ECS AccessDeniedException error messages aren't displayed
- The fix is proposed in PR #2860, linked to issue #2859
- The fork's commit (a8afd4a) contains minimal changes to fix the error message deserialization
- The TODO comment properly documents both the temporary nature and tracking issue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the fork is ahead/behind the original
echo "Checking fork status..."
gh repo view aws/aws-sdk-go-v2 --json url -q .url | xargs -I {} gh api "repos/aws/aws-sdk-go-v2/compare/main...abicky:aws-sdk-go-v2:main" || true
# Check if the issue is still open
echo "Checking issue status..."
gh issue view 2859 --repo aws/aws-sdk-go-v2 --json state -q .state || true
Length of output: 3779
Script:
#!/bin/bash
# Check the commit referenced in the replace directive
echo "Checking specific commit changes..."
gh api repos/abicky/aws-sdk-go-v2/commits/a8afd4a537b1 || true
# Check if there are any PRs from abicky to aws/aws-sdk-go-v2
echo "Checking related PRs..."
gh pr list --repo aws/aws-sdk-go-v2 --author abicky --json title,url,state || true
Length of output: 5748
cmd/terminatespotfleetinstances.go (5)
4-5: LGTM: AWS SDK v2 imports.
The import paths have been correctly updated to use AWS SDK v2 packages.
39-52: LGTM: AWS client initialization with proper error handling.
The AWS client initialization has been correctly updated to use AWS SDK v2 patterns:
- Using context-aware configuration
- Proper error handling for both configuration and client initialization
- Following the NewFromConfig pattern for service clients
54-57: LGTM: Context-aware method call.
The TerminateAllInstances call has been correctly updated to include context propagation with proper error handling.
29-29: Verify batch size constant compatibility.
The flag type has been changed from Int64 to Int32. Let's verify that the constant value from ecsconst.MaxListableContainerInstances fits within Int32 bounds and check for any other usage of this constant.
✅ Verification successful
Batch size constant value is compatible with Int32
The constant MaxListableContainerInstances is set to 100, which is well within Int32 bounds. The change from Int64 to Int32 is safe and aligns with AWS SDK v2's conventions. All usages of this constant are consistent across the codebase:
- Used as batch size flag in multiple commands
- Used in capacity drainer with proper validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the constant value and its usage
# Expected: Find the constant definition and all its usages
# Search for the constant definition
rg -A 1 "MaxListableContainerInstances.*=" internal/const/ecsconst/
# Search for all usages of this constant
rg "MaxListableContainerInstances"
Length of output: 1072
19-19: Verify spot fleet request state check.
The command description specifies that it works with spot fleet requests in "cancelled" state. Let's verify this requirement is enforced in the code.
✅ Verification successful
Based on the results, I can now generate the final response since I have found the relevant code that handles the spot fleet request state validation.
Spot fleet request state validation is correctly implemented
The code in internal/capacity/spotfleetrequest.go properly validates the spot fleet request state by checking if it starts with "cancelled" (which covers both "cancelled" and "cancelled_running" states). The validation is enforced with an appropriate error message when the state requirement is not met.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the spot fleet request state is validated
# Expected: Find code that verifies the spot fleet request is in "cancelled" state
# Search in the capacity package for spot fleet state validation
rg -A 5 "cancelled" internal/capacity/
ast-grep --pattern 'if $_ == "cancelled" { $$$_ }'
Length of output: 2083
internal/service/definition.go (3)
4-11: LGTM: Import statements correctly updated for AWS SDK v2
The transition from AWS SDK v1 to v2 has been properly implemented in the import statements.
64-70: LGTM: Simple and correct implementations
The merge and buildCreateServiceInput methods are implemented correctly:
- merge properly uses mergo for deep copying with override
- buildCreateServiceInput safely converts between aliased types
9-11: Verify AWS SDK v2 migration completeness
Let's ensure all AWS SDK imports have been migrated to v2 consistently across the codebase.
✅ Verification successful
Let me try another script to properly check for AWS SDK v1 imports:
AWS SDK v2 migration is complete and consistent
The verification shows that all AWS SDK imports across the codebase are using v2 consistently. Every import starts with github.com/aws/aws-sdk-go-v2/ and there are no v1 SDK imports remaining. The ecstypes alias is also used consistently across all files that need the ECS types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining AWS SDK v1 imports that need migration
# Test 1: Look for any remaining v1 SDK imports
echo "Checking for AWS SDK v1 imports..."
rg "github.com/aws/aws-sdk-go(?!/v2)" -l
# Test 2: Verify consistent use of ecstypes
echo -e "\nChecking for consistent use of ecstypes..."
rg "github.com/aws/aws-sdk-go-v2/service/ecs/types" -C 2
Length of output: 2572
Script:
#!/bin/bash
# Description: Check for any remaining AWS SDK v1 imports using a different approach
# Test 1: Look for any remaining v1 SDK imports
echo "Checking for AWS SDK v1 imports..."
rg '"github.com/aws/aws-sdk-go/' --type go | grep -v '"github.com/aws/aws-sdk-go-v2/'
# Test 2: Look for any v1 specific types like "aws/session"
echo -e "\nChecking for v1 specific imports..."
rg '"github.com/aws/aws-sdk-go/aws/session' --type go
# Test 3: Check all AWS SDK imports for a complete picture
echo -e "\nListing all AWS SDK imports..."
rg '^[[:space:]]*"github.com/aws/aws-sdk-go' --type go
Length of output: 3760
internal/capacity/spotfleetrequest.go (5)
10-13: LGTM: AWS SDK v2 imports and type updates are correct.
The migration from AWS SDK v1 to v2 has been properly implemented with appropriate import paths and type updates.
Also applies to: 18-22
44-45: LGTM: AWS SDK v2 API usage is correct.
The code properly uses SDK v2 types for fleet type comparisons and EC2 API calls. The waiter pattern is also correctly implemented.
Also applies to: 53-65
86-96: LGTM: Atomic operations are properly implemented.
The code correctly uses atomic operations with proper synchronization through atomic.Int32. The previous concerns about race conditions have been addressed, as confirmed in past reviews.
91-98: LGTM: Error handling strategy is appropriate.
The error handling in the Poll callback is correct. As confirmed in past reviews, errors are not critical since the process retries and the main function has a timeout.
26-31: 🛠️ Refactor suggestion
Consider passing context as a parameter to NewSpotFleetRequest.
Currently using context.Background() which prevents the caller from controlling timeouts and cancellation. Consider propagating the context from the caller.
-func NewSpotFleetRequest(id string, ec2Svc EC2API) (*SpotFleetRequest, error) {
+func NewSpotFleetRequest(ctx context.Context, id string, ec2Svc EC2API) (*SpotFleetRequest, error) {
sfr := SpotFleetRequest{ec2Svc: ec2Svc, id: id}
- if err := sfr.reload(context.Background()); err != nil {
+ if err := sfr.reload(ctx); err != nil {
return nil, err
}
return &sfr, nil
}Likely invalid or redundant comment.
internal/service/service_test.go (3)
4-5: LGTM! Import and setup changes are correct.
The changes appropriately:
- Replace deprecated
ioutil.Discardwithio.Discard - Update imports to use AWS SDK v2 packages
Also applies to: 11-14, 22-22
158-163: LGTM! Test cases have been correctly updated for AWS SDK v2.
The changes appropriately:
- Add context handling
- Update to SDK v2 types
- Maintain test coverage for both success and error scenarios
Also applies to: 174-183, 187-187, 197-205, 209-209, 215-246
176-176: Verify AWS SDK v2 client initialization.
Since this is part of the AWS SDK v2 migration, ensure that the ECS client is properly initialized with the correct configuration (e.g., retry options, timeouts) in the main code.
Also applies to: 238-238
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about the ECS client initialization and usage in the codebase:
ECS client initialization is properly configured
The ECS client is correctly initialized in the codebase with appropriate configuration:
- The client is initialized using
ecs.NewFromConfig(cfg)which inherits AWS SDK v2 configuration - Proper timeouts and retry options are set for waiters:
- ServicesStableWaiter: 15 seconds max delay
- TasksStoppedWaiter: 6 seconds max delay
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ECS client initialization to verify proper configuration
rg -A 5 "New.*Client.*ecs"
Length of output: 28
Script:
#!/bin/bash
# Let's try different search patterns to find ECS client initialization
# Search for ECS client type and initialization
rg -A 5 "type.*ECS|NewECSAPI|ecs\.New"
# Also search for mock initialization to understand the context better
rg -A 5 "NewMockECSAPI"
# Search for any ECS-related imports
rg "ecs|aws-sdk-go-v2/service/ecs"
Length of output: 46161
internal/capacity/drainer.go (2)
4-15: LGTM: AWS SDK v2 migration changes
The changes properly implement the AWS SDK v2 migration by:
- Adding context support to interface methods
- Using appropriate AWS SDK v2 types
- Updating imports to use v2 packages
Also applies to: 22-23, 28-29
12-15: Verify complete migration to AWS SDK v2
Let's verify that all AWS SDK v1 imports have been properly migrated to v2.
✅ Verification successful
AWS SDK v2 migration is complete
The verification confirms that all AWS SDK imports are using v2, with no remaining v1 imports. The codebase consistently uses AWS SDK v2 packages across all files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining AWS SDK v1 imports
# Test: Search for aws-sdk-go v1 imports. Expect: No matches
echo "Checking for remaining AWS SDK v1 imports..."
rg "github.com/aws/aws-sdk-go(/|$)" || echo "No AWS SDK v1 imports found"
echo "Verifying AWS SDK v2 imports..."
rg "github.com/aws/aws-sdk-go-v2(/|$)"
Length of output: 10614
internal/capacity/autoscalinggroup.go (3)
4-30: LGTM! AWS SDK v2 migration looks good.
The changes to imports and struct definitions correctly implement the AWS SDK v2 migration. Notable improvements include:
- Using the new AWS SDK v2 packages and types
- Leveraging the standard library's
slicespackage - Proper type alignment with AWS SDK v2 specifications
Line range hint 42-75: LGTM! Core methods properly handle context and error wrapping.
The implementation of ReplaceInstances and ReduceCapacity correctly:
- Propagates context through the call chain
- Maintains proper error wrapping with descriptive messages
- Handles instance lifecycle management appropriately
78-106: LGTM! Helper methods properly implement AWS SDK v2 patterns.
The implementation correctly:
- Uses AWS SDK v2 types and APIs
- Handles errors with proper context
- Uses value types instead of pointers where appropriate
Also applies to: 119-137, 299-307
internal/service/service.go (4)
59-59: Good use of predefined constant for ServiceField
Great job replacing the string literal with the predefined constant ecstypes.ServiceFieldTags for the Include field. This enhances type safety and reduces the risk of typos.
78-78: Ensure no sensitive information is logged
The log.Printf statement at line 78 logs the entire service definition def. This may inadvertently expose sensitive information such as configuration details or environment variables. Please verify that def does not contain any sensitive data before logging.
127-127: Good use of predefined constant for DesiredStatus
Well done updating DesiredStatus to use the predefined constant ecstypes.DesiredStatusRunning. This improves code clarity and ensures consistency with AWS SDK v2 constants.
184-185: Update error handling to match AWS SDK v2 conventions
As previously noted, the error type *ecstypes.InvalidParameterException may not be accurate for AWS SDK v2. Additionally, accessing the error message might differ. Please adjust the error handling to align with AWS SDK v2 practices.
| ) | ||
|
|
||
| type Definition ecs.CreateServiceInput | ||
|
|
||
| func NewDefinitionFromExistingService(s *ecs.Service) *Definition { | ||
| func NewDefinitionFromExistingService(s ecstypes.Service) *Definition { |
There was a problem hiding this comment.
Add validation for deployments slice access
Direct access to s.Deployments[0] could panic if the deployments slice is empty. Consider adding validation:
func NewDefinitionFromExistingService(s ecstypes.Service) *Definition {
+ if len(s.Deployments) == 0 {
+ return nil
+ }
// ... rest of the function
}Also applies to: 50-50, 54-54
| func (d *Definition) GoString() string { | ||
| var sb strings.Builder | ||
| d.prettify(&sb, reflect.ValueOf(d), 0) | ||
| return sb.String() | ||
| } | ||
|
|
||
| func (d *Definition) merge(other *Definition) error { | ||
| return mergo.Merge(d, *other, mergo.WithOverride) | ||
| } | ||
|
|
||
| func (d *Definition) buildCreateServiceInput() *ecs.CreateServiceInput { | ||
| return (*ecs.CreateServiceInput)(d) | ||
| } | ||
|
|
||
| func (d *Definition) prettify(sb *strings.Builder, v reflect.Value, indent int) { | ||
| if v.Kind() == reflect.Ptr { | ||
| v = v.Elem() | ||
| } | ||
|
|
||
| switch v.Kind() { | ||
| case reflect.Struct: | ||
| sb.WriteString("{\n") | ||
| for i := 0; i < v.NumField(); i++ { | ||
| if v.Type().Field(i).PkgPath != "" { | ||
| continue | ||
| } | ||
|
|
||
| f := v.Field(i) | ||
| if (f.Kind() == reflect.Ptr || f.Kind() == reflect.Slice) && f.IsNil() { | ||
| continue | ||
| } | ||
|
|
||
| sb.WriteString(strings.Repeat(" ", indent+1)) | ||
| sb.WriteString(v.Type().Field(i).Name) | ||
| sb.WriteString(": ") | ||
| d.prettify(sb, v.Field(i), indent+1) | ||
| sb.WriteString(",\n") | ||
| } | ||
| sb.WriteString(strings.Repeat(" ", indent)) | ||
| sb.WriteString("}") | ||
| case reflect.Slice: | ||
| if v.Len() == 0 { | ||
| sb.WriteString("[]") | ||
| return | ||
| } | ||
|
|
||
| sb.WriteString("[\n") | ||
| for i := 0; i < v.Len(); i++ { | ||
| sb.WriteString(strings.Repeat(" ", indent+1)) | ||
| d.prettify(sb, v.Index(i), indent+1) | ||
| sb.WriteString(",\n") | ||
| } | ||
| sb.WriteString(strings.Repeat(" ", indent)) | ||
| sb.WriteString("]") | ||
| default: | ||
| if v.Kind() == reflect.String { | ||
| fmt.Fprintf(sb, "%q", v.String()) | ||
| } else { | ||
| fmt.Fprint(sb, v.Interface()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance optimizations for string building
The string building implementation is correct but could be optimized:
- Pre-allocate the StringBuilder capacity based on an estimated size
- Cache reflect.Value.Type() calls in struct handling
func (d *Definition) GoString() string {
- var sb strings.Builder
+ sb := strings.Builder{}
+ // Estimate initial capacity to reduce allocations
+ sb.Grow(1024)
d.prettify(&sb, reflect.ValueOf(d), 0)
return sb.String()
}
func (d *Definition) prettify(sb *strings.Builder, v reflect.Value, indent int) {
// ... at line 78
case reflect.Struct:
+ t := v.Type() // Cache type to avoid repeated calls
sb.WriteString("{\n")
for i := 0; i < v.NumField(); i++ {
- if v.Type().Field(i).PkgPath != "" {
+ if t.Field(i).PkgPath != "" {
continue
}
// ... rest of the struct handling
- sb.WriteString(v.Type().Field(i).Name)
+ sb.WriteString(t.Field(i).Name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (d *Definition) GoString() string { | |
| var sb strings.Builder | |
| d.prettify(&sb, reflect.ValueOf(d), 0) | |
| return sb.String() | |
| } | |
| func (d *Definition) merge(other *Definition) error { | |
| return mergo.Merge(d, *other, mergo.WithOverride) | |
| } | |
| func (d *Definition) buildCreateServiceInput() *ecs.CreateServiceInput { | |
| return (*ecs.CreateServiceInput)(d) | |
| } | |
| func (d *Definition) prettify(sb *strings.Builder, v reflect.Value, indent int) { | |
| if v.Kind() == reflect.Ptr { | |
| v = v.Elem() | |
| } | |
| switch v.Kind() { | |
| case reflect.Struct: | |
| sb.WriteString("{\n") | |
| for i := 0; i < v.NumField(); i++ { | |
| if v.Type().Field(i).PkgPath != "" { | |
| continue | |
| } | |
| f := v.Field(i) | |
| if (f.Kind() == reflect.Ptr || f.Kind() == reflect.Slice) && f.IsNil() { | |
| continue | |
| } | |
| sb.WriteString(strings.Repeat(" ", indent+1)) | |
| sb.WriteString(v.Type().Field(i).Name) | |
| sb.WriteString(": ") | |
| d.prettify(sb, v.Field(i), indent+1) | |
| sb.WriteString(",\n") | |
| } | |
| sb.WriteString(strings.Repeat(" ", indent)) | |
| sb.WriteString("}") | |
| case reflect.Slice: | |
| if v.Len() == 0 { | |
| sb.WriteString("[]") | |
| return | |
| } | |
| sb.WriteString("[\n") | |
| for i := 0; i < v.Len(); i++ { | |
| sb.WriteString(strings.Repeat(" ", indent+1)) | |
| d.prettify(sb, v.Index(i), indent+1) | |
| sb.WriteString(",\n") | |
| } | |
| sb.WriteString(strings.Repeat(" ", indent)) | |
| sb.WriteString("]") | |
| default: | |
| if v.Kind() == reflect.String { | |
| fmt.Fprintf(sb, "%q", v.String()) | |
| } else { | |
| fmt.Fprint(sb, v.Interface()) | |
| } | |
| } | |
| } | |
| func (d *Definition) GoString() string { | |
| sb := strings.Builder{} | |
| // Estimate initial capacity to reduce allocations | |
| sb.Grow(1024) | |
| d.prettify(&sb, reflect.ValueOf(d), 0) | |
| return sb.String() | |
| } | |
| func (d *Definition) merge(other *Definition) error { | |
| return mergo.Merge(d, *other, mergo.WithOverride) | |
| } | |
| func (d *Definition) buildCreateServiceInput() *ecs.CreateServiceInput { | |
| return (*ecs.CreateServiceInput)(d) | |
| } | |
| func (d *Definition) prettify(sb *strings.Builder, v reflect.Value, indent int) { | |
| if v.Kind() == reflect.Ptr { | |
| v = v.Elem() | |
| } | |
| switch v.Kind() { | |
| case reflect.Struct: | |
| t := v.Type() // Cache type to avoid repeated calls | |
| sb.WriteString("{\n") | |
| for i := 0; i < v.NumField(); i++ { | |
| if t.Field(i).PkgPath != "" { | |
| continue | |
| } | |
| f := v.Field(i) | |
| if (f.Kind() == reflect.Ptr || f.Kind() == reflect.Slice) && f.IsNil() { | |
| continue | |
| } | |
| sb.WriteString(strings.Repeat(" ", indent+1)) | |
| sb.WriteString(t.Field(i).Name) | |
| sb.WriteString(": ") | |
| d.prettify(sb, v.Field(i), indent+1) | |
| sb.WriteString(",\n") | |
| } | |
| sb.WriteString(strings.Repeat(" ", indent)) | |
| sb.WriteString("}") | |
| case reflect.Slice: | |
| if v.Len() == 0 { | |
| sb.WriteString("[]") | |
| return | |
| } | |
| sb.WriteString("[\n") | |
| for i := 0; i < v.Len(); i++ { | |
| sb.WriteString(strings.Repeat(" ", indent+1)) | |
| d.prettify(sb, v.Index(i), indent+1) | |
| sb.WriteString(",\n") | |
| } | |
| sb.WriteString(strings.Repeat(" ", indent)) | |
| sb.WriteString("]") | |
| default: | |
| if v.Kind() == reflect.String { | |
| fmt.Fprintf(sb, "%q", v.String()) | |
| } else { | |
| fmt.Fprint(sb, v.Interface()) | |
| } | |
| } | |
| } |
| paginator := ecs.NewListTasksPaginator(d.ecsSvc, params) | ||
| for paginator.HasMorePages() { | ||
| page, err := paginator.NextPage(ctx) | ||
| if err != nil { | ||
| return xerrors.Errorf("failed to list tasks: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in pagination
The error handling could be more specific about which page failed during pagination.
paginator := ecs.NewListTasksPaginator(d.ecsSvc, params)
+ pageNum := 0
for paginator.HasMorePages() {
+ pageNum++
page, err := paginator.NextPage(ctx)
if err != nil {
- return xerrors.Errorf("failed to list tasks: %w", err)
+ return xerrors.Errorf("failed to list tasks (page %d): %w", pageNum, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| paginator := ecs.NewListTasksPaginator(d.ecsSvc, params) | |
| for paginator.HasMorePages() { | |
| page, err := paginator.NextPage(ctx) | |
| if err != nil { | |
| return xerrors.Errorf("failed to list tasks: %w", err) | |
| } | |
| paginator := ecs.NewListTasksPaginator(d.ecsSvc, params) | |
| pageNum := 0 | |
| for paginator.HasMorePages() { | |
| pageNum++ | |
| page, err := paginator.NextPage(ctx) | |
| if err != nil { | |
| return xerrors.Errorf("failed to list tasks (page %d): %w", pageNum, err) | |
| } |
| tasksStoppedWaiter := ecs.NewTasksStoppedWaiter(d.ecsSvc, func(o *ecs.TasksStoppedWaiterOptions) { | ||
| o.MaxDelay = 6 * time.Second | ||
| }) | ||
| for arns := range slices.Chunk(allTaskArns, ecsconst.MaxDescribableTasks) { | ||
| err := tasksStoppedWaiter.Wait(ctx, &ecs.DescribeTasksInput{ | ||
| Cluster: aws.String(d.cluster), | ||
| Tasks: arns, | ||
| }) | ||
| }, 10*time.Minute) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making waiter configurations configurable
The waiter configurations (MaxDelay and timeout) are hardcoded. Consider making these configurable through the Drainer interface to allow for different scenarios and requirements.
type drainer struct {
cluster string
batchSize int32
ecsSvc ECSAPI
+ taskWaiterConfig struct {
+ maxDelay time.Duration
+ timeout time.Duration
+ }
+ serviceWaiterConfig struct {
+ maxDelay time.Duration
+ timeout time.Duration
+ }
}Committable suggestion was skipped due to low confidence.
| waiter := ec2.NewInstanceTerminatedWaiter(asg.ec2Svc, func(o *ec2.InstanceTerminatedWaiterOptions) { | ||
| o.MaxDelay = 15 * time.Second | ||
| }) | ||
| err = waiter.Wait(ctx, &ec2.DescribeInstancesInput{ | ||
| InstanceIds: sortedInstanceIDs, | ||
| }, 10*time.Minute) | ||
| if err != nil { | ||
| return xerrors.Errorf("failed to terminate the instances: %w", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making waiter configuration more flexible.
The instance termination waiter uses hardcoded values for timeout and delay. Consider making these configurable:
- Extract timeout duration (10 minutes) to a configurable constant
- Allow waiter options to be customized through configuration
+const (
+ DefaultInstanceTerminationTimeout = 10 * time.Minute
+ DefaultWaiterMaxDelay = 15 * time.Second
+)
waiter := ec2.NewInstanceTerminatedWaiter(asg.ec2Svc, func(o *ec2.InstanceTerminatedWaiterOptions) {
- o.MaxDelay = 15 * time.Second
+ o.MaxDelay = DefaultWaiterMaxDelay
})
err = waiter.Wait(ctx, &ec2.DescribeInstancesInput{
InstanceIds: sortedInstanceIDs,
-}, 10*time.Minute)
+}, DefaultInstanceTerminationTimeout)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| waiter := ec2.NewInstanceTerminatedWaiter(asg.ec2Svc, func(o *ec2.InstanceTerminatedWaiterOptions) { | |
| o.MaxDelay = 15 * time.Second | |
| }) | |
| err = waiter.Wait(ctx, &ec2.DescribeInstancesInput{ | |
| InstanceIds: sortedInstanceIDs, | |
| }, 10*time.Minute) | |
| if err != nil { | |
| return xerrors.Errorf("failed to terminate the instances: %w", err) | |
| const ( | |
| DefaultInstanceTerminationTimeout = 10 * time.Minute | |
| DefaultWaiterMaxDelay = 15 * time.Second | |
| ) | |
| waiter := ec2.NewInstanceTerminatedWaiter(asg.ec2Svc, func(o *ec2.InstanceTerminatedWaiterOptions) { | |
| o.MaxDelay = DefaultWaiterMaxDelay | |
| }) | |
| err = waiter.Wait(ctx, &ec2.DescribeInstancesInput{ | |
| InstanceIds: sortedInstanceIDs, | |
| }, DefaultInstanceTerminationTimeout) | |
| if err != nil { | |
| return xerrors.Errorf("failed to terminate the instances: %w", err) |
abicky
force-pushed
the
upgrade-go-and-packages
branch
from
338daee
to
a948525
Compare
abicky
force-pushed
the
upgrade-go-and-packages
branch
from
a948525
to
cdccb4e
Compare
This PR upgrades the following versions:
Summary by CodeRabbit
Release Notes
New Features
AutoScalingAPI,EC2API,ECSAPI, andSQSAPI.ecsmecCLI tool to clarify binary downloads for macOS Apple silicon.terminate-spot-fleet-request-instancestoterminate-spot-fleet-instancesfor clarity.Bug Fixes
iam:PassRole.Refactor
Chores
go.modto reflect the new AWS SDK and other libraries.