Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter active cluster before calling active services #5204

Merged
merged 2 commits into from
Aug 18, 2023

Conversation

KollaAdithya
Copy link
Contributor

@KollaAdithya KollaAdithya commented Aug 18, 2023

address #5125 (comment)
This PR fixes below
Scenario


App: demo
Env: test
Svc: frontend

  1. And the svc is successfully deployed , copilot svc show and copilot task run —--generate-cmd demo/test/frontend works ✅
Cluster - demo-test-Cluster-ABC(Active

Service - demo-test-Cluster-ABC/demo-test-frontend-Service-ABC(Active)

  1. After this if i delete the test environment and frontend service.

Cluster - demo-test-Cluster-ABC (InActive) 

Service - demo-test-Cluster-ABC/demo-test-frontend-Service-ABC(InActive)

  1. Deploying service and Environment with the same names.
Cluster - demo-test-Cluster-ABC(InActive) 

Service - demo-test-Cluster-ABC/demo-test-frontend-Service-ABC(InActive)


Cluster - demo-test-Cluster-DEF(Active) 

Service - demo-test-Cluster-DEF/demo-test-frontend-Service-DEF(Active)
  1. copilot svc show and copilot task run —--generate-cmd demo/test/frontend will generate the below errors.



copilot task run --generate-cmd demo/test/frontend
✘ generate task run command from service frontend of application demo deployed in environment test: retrieve network configuration for service frontend: check if services are active: service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-Bus8o0uclnAW/demo-test-frontend-Service-E0rKy3e2S78t" and service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-TJyweayOGepM/demo-test-frontend-Service-4Se9IVA2878D" should be in the same cluster



copilot svc show                                
Application: demo
Only found one service, defaulting to: frontend
✘ describe service frontend: retrieve rollback alarm names: get service frontend: check if services are active: service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-Bus8o0uclnAW/demo-test-frontend-Service-E0rKy3e2S78t" and service "arn:aws:ecs:us-west-2:197732814171:service/demo-test-Cluster-TJyweayOGepM/demo-test-frontend-Service-4Se9IVA2878D" should be in the same cluster





By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@KollaAdithya KollaAdithya requested a review from a team as a code owner August 18, 2023 15:35
@KollaAdithya KollaAdithya requested review from CaptainCarpensir and removed request for a team August 18, 2023 15:35
@github-actions
Copy link

github-actions bot commented Aug 18, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 51624 51488 +0.26
macOS (arm) 51824 51680 +0.28
linux (amd) 45444 45324 +0.26
linux (arm) 43716 43588 +0.29
windows (amd) 42252 42144 +0.26

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Merging #5204 (248dcc2) into mainline (86efee6) will decrease coverage by 0.02%.
The diff coverage is 73.07%.

@@             Coverage Diff              @@
##           mainline    #5204      +/-   ##
============================================
- Coverage     69.52%   69.51%   -0.02%     
============================================
  Files           295      295              
  Lines         43897    43907      +10     
  Branches        285      285              
============================================
+ Hits          30518    30520       +2     
- Misses        11889    11895       +6     
- Partials       1490     1492       +2     
Files Changed Coverage Δ
internal/pkg/aws/ecs/ecs.go 90.31% <72.22%> (-0.78%) ⬇️
internal/pkg/ecs/ecs.go 74.73% <75.00%> (-0.55%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

looks great!

internal/pkg/aws/ecs/ecs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@mergify mergify bot merged commit 77c88c2 into aws:mainline Aug 18, 2023
12 checks passed
Sprint 🏃‍♀️ automation moved this from In review to Pending release Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
Pending release
Development

Successfully merging this pull request may close these issues.

None yet

4 participants