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

refactor(cli): apply deploy store APIs to svc logs #1096

Closed

Conversation

iamhopaul123
Copy link
Contributor

@iamhopaul123 iamhopaul123 commented Jul 8, 2020

Part of #1069.

Note that parsing service name by calling ecs API to get the service tag costs 1.60 seconds on average, while using regex to get service name from its ARN costs 1.55 seconds on average. (one service and one environment. The difference might be a little bit more obvious if there are more than 5 combinations)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@iamhopaul123 iamhopaul123 requested a review from a team as a code owner July 8, 2020 01:50
@iamhopaul123 iamhopaul123 added the WIP Pull requests that are being modified now. label Jul 8, 2020
@iamhopaul123 iamhopaul123 marked this pull request as draft July 8, 2020 01:51
// // getServiceName gets the ECS service name given a specific ARN.
// // For example: arn:aws:ecs:us-west-2:123456789012:service/my-app-test-Cluster-cdgG2k6XIBtM/my-app-test-my-svc-Service-WLYA7MGACV1F
// // returns my-service
// func (s *Store) getServiceName(svcARN string) (string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use ECS API to get all tags for ECS service allows us to get a "safer" service name. However, it requires calling ECS get tags API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can modify rgClient.GetResourcesByTags to return to us:

[{
    arn: string,
    tags: map[string]string,
}]

and tap into tags[ServiceTagKey] to get the name of the svc

return "", fmt.Errorf(`cannot parse service ARN resource "%s"`, resp.Resource)
// For example: arn:aws:ecs:us-west-2:123456789012:service/my-app-test-Cluster-cdgG2k6XIBtM/my-app-test-my-svc-Service-WLYA7MGACV1F
// returns my-service
func (s *Store) getServiceName(svcARN, appName, envName string) (string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using regex could potentially be cheaper than calling ECS API to get service name directly from the service ARN. However, it could be not that safe to do so because it is regex.

@iamhopaul123 iamhopaul123 requested a review from bvtujo July 8, 2020 02:05
@iamhopaul123 iamhopaul123 changed the title fix(cli): apply deploy store APIs to svc logs refactor(cli): apply deploy store APIs to svc logs Jul 8, 2020
Comment on lines +123 to +125
if tag.Key != nil && tag.Value != nil {
tags[*tag.Key] = *tag.Value
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the key has to be not nil but the value can be nil :/

Each tag consists of a key and an optional value, both of which you define.
(https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html)

So

Suggested change
if tag.Key != nil && tag.Value != nil {
tags[*tag.Key] = *tag.Value
}
if tag.Key == nil {
continue
}
key := *tag.Key
value := ""
if tag.Value != nil {
value = *tag.Value
}

@@ -76,6 +87,66 @@ func NewWorkspaceSelect(prompt Prompter, store *config.Store, ws *workspace.Work
}
}

// NewDeploySelect returns a new selector that chooses services and environments from the deploy store.
func NewDeploySelect(prompt Prompter, configStore *config.Store, deployStore *deploy.Store) *DeploySelect {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is my bad, but can we take as input interfaces to the constructors instead of structs?

@@ -51,6 +56,12 @@ type WorkspaceSelect struct {
svcLister wsSvcLister
}

// DeploySelect is a service and environment selector from the deploy store.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe:

Suggested change
// DeploySelect is a service and environment selector from the deploy store.
// DeploySelect prompts the users to select the name of a **deployed** service or environment.

initCwLogsSvc: func(o *svcLogsOpts, envName string) error {
env, err := o.configStore.GetEnvironment(o.AppName(), envName)
if err != nil {
return fmt.Errorf("get envronment: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo :D

Suggested change
return fmt.Errorf("get envronment: %w", err)
return fmt.Errorf("get environment config: %w", err)

svcLogNameHelpPrompt,
svcEnvNames,
)
svcName, envName, err := o.sel.ServiceEnvironment(svcLogNamePrompt, svcLogNameHelpPrompt, o.AppName())
Copy link
Contributor

Choose a reason for hiding this comment

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

#blessed

}

// NewStore returns a new store.
func NewStore(store ConfigStoreClient) (*Store, error) {
s := &Store{
configStore: store,
}
s.newRgClientFromIDs = func(appName, envName string) error {
s.newClientFromIDs = func(appName, envName string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe

Suggested change
s.newClientFromIDs = func(appName, envName string) error {
s.newClientsFromEnv = func(appName, envName string) error {

s.rgClient = rg.New(sess)
return nil
}
s.newRgClientFromRole = func(roleARN, region string) error {
s.newClientFromRole = func(roleARN, region string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
s.newClientFromRole = func(roleARN, region string) error {
s.newClientsFromRole = func(roleARN, region string) error {

// // getServiceName gets the ECS service name given a specific ARN.
// // For example: arn:aws:ecs:us-west-2:123456789012:service/my-app-test-Cluster-cdgG2k6XIBtM/my-app-test-my-svc-Service-WLYA7MGACV1F
// // returns my-service
// func (s *Store) getServiceName(svcARN string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can modify rgClient.GetResourcesByTags to return to us:

[{
    arn: string,
    tags: map[string]string,
}]

and tap into tags[ServiceTagKey] to get the name of the svc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Pull requests that are being modified now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants