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.

Sign up for GitHub

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

Jump to bottom

feat: check if service is available in the region #3326

Merged
merged 7 commits into from
Mar 15, 2022
Merged

feat: check if service is available in the region #3326

merged 7 commits into from
Mar 15, 2022

Conversation

paragbhingre
Copy link
Contributor

This PR adds ability to check if service exist in the given target environment region for every deploy command.

This PR resolves #3172

@paragbhingre paragbhingre requested a review from a team as a code owner March 3, 2022 23:54
@paragbhingre paragbhingre requested review from dannyrandall and removed request for a team March 3, 2022 23:54
@paragbhingre
Copy link
Contributor Author

paragbhingre commented Mar 4, 2022 •
edited
Loading

Oops some test errors, while I check that you can still take a look at implementation 😁

@Lou1415926 Lou1415926 changed the title fix: Check if Service is available in the region feat: Check if Service is available in the region Mar 4, 2022
efekarakus
efekarakus reviewed Mar 4, 2022
View reviewed changes
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

I haven't looked deeply to the PR, but at a glance it looks great!
Just some minor feedback, feel free to not wait for my approval if others approve.

internal/pkg/manifest/lb_web_svc.go Outdated
@@ -327,3 +329,7 @@ func (e *Alias) ToString() string {
}
return strings.Join(e.StringSlice, ",")
}

func (s *LoadBalancedWebService) IsServiceAvailableInRegion(region string) (bool, error) {
return regions.IsServiceAvailable(ecs.EndpointsID, region)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the constants to our internal/aws pkgs for example:

// in internal/pkg/aws/ecs
package ecs

const ServiceID = ecs.ServiceID

We want to do this because it makes it easier to upgrade a dependency (aws-sdk-go) from v1 to v2. This way we have a single location to update internal/pkg/aws instead of updating multiple pkgs in the codebase. Hope that makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

(also I believe it needs to be .ServiceID instead of .EndpointsID)

Copy link
Contributor

@Lou1415926 Lou1415926 Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I believe EndpointsID is used here because of this description in the aws-sdk-go doc:

Deprecated: Use client package's EndpointsID value instead of these ServiceIDs. These IDs are not maintained, and are out of date.

Very confusing description since the parameter names are still ServiceID 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and tested it -
apprunner package's ServiceID is "AppRunner", and its EndpointsID is apprunner. The ServiceID when passed into endpoints.RegionsForService doesn't work, while the latter does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes even I did that testing and ServiceID did not work where as EndpintsID worked, hence used EndpointsID finally.

Lou1415926
Lou1415926 reviewed Mar 4, 2022
View reviewed changes
internal/pkg/manifest/backend_svc_test.go Outdated
"region exist": {
region: "us-west-1",
want: true,
wantErr: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably redundant since wantErr is initialized as nil.

Suggested change
wantErr: nil,

Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually not clear what these unit tests in manifest package are testing 🤔 These tests seems to be testing regions.IsServiceAvailable, instead of manifest's IsServiceAvailable

Copy link
Contributor Author

@paragbhingre paragbhingre Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah right, first I thought it is important to have a unit test for each kind of a service for example loadbalanced service or backend service and etc. But then I realized it is enough to write a test for regions.IsServiceAvailable with only two scenarios i.e. ecs service and apprunner service. So I have removed tests from other service_test files and have only one test for regions.IsServiceAvailable
Let me know if that makes sense?

internal/pkg/aws/regions/regions.go Outdated
)

// IsServiceAvailable returns true if the service ID is available in the given region.
func IsServiceAvailable(sid string, region string) (bool, error) {
Copy link
Contributor

@Lou1415926 Lou1415926 Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we have unit tests for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as per above comment

huanjani
huanjani reviewed Mar 4, 2022
View reviewed changes
Copy link
Contributor

@huanjani huanjani left a comment

Choose a reason for hiding this comment

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

This feature is going to be so helpful for our users! A couple small comments before I look again more deeply.

@efekarakus efekarakus changed the title feat: Check if Service is available in the region feat: check if service is available in the region Mar 8, 2022
iamhopaul123
iamhopaul123 reviewed Mar 8, 2022
View reviewed changes
Lou1415926
Lou1415926 reviewed Mar 8, 2022
View reviewed changes
@Lou1415926 Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 14, 2022
Lou1415926
Lou1415926 approved these changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

Awesome feature! Feel free to remove the DNM label when you are ready.

@paragbhingre paragbhingre removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 15, 2022
@mergify mergify bot merged commit 1095529 into aws:mainline Mar 15, 2022
@paragbhingre paragbhingre deleted the appRunnerRegion3172 branch January 26, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efekarakus efekarakus efekarakus left review comments

@huanjani huanjani huanjani left review comments

@iamhopaul123 iamhopaul123 iamhopaul123 approved these changes

@Lou1415926 Lou1415926 Lou1415926 approved these changes

@dannyrandall dannyrandall Awaiting requested review from dannyrandall dannyrandall is a code owner automatically assigned from aws/aws-ecs-devx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check whether AppRunner is available in the region
5 participants
@paragbhingre @efekarakus @iamhopaul123 @huanjani @Lou1415926