Skip to content

Conversation

@huanjani
Copy link
Contributor

@huanjani huanjani commented Mar 12, 2020

Prepended the protocol to the url string for a more user-friendly output for 'app show'.

Included changes in go.mod and go.sum because of advice given here: https://github.com/golang/go/wiki/Modules#should-i-commit-my-gosum-file-as-well-as-my-gomod-file.

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

@huanjani huanjani requested a review from a team as a code owner March 12, 2020 00:04
@huanjani huanjani requested a review from kohidave March 12, 2020 00:04
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.

Mostly LGTM! Just few questions.

Copy link
Contributor

@SoManyHs SoManyHs left a comment

Choose a reason for hiding this comment

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

This looks great!! 🎉 Could you also add output from manually testing the app show command plz? :)

@efekarakus efekarakus added the area/svc Issues about services. label Mar 12, 2020
@kohidave
Copy link
Contributor

Could you also update init_test.go, line 96 in the e2e tests? From TrimSuffix to strings.TrimPrefix(app.Routes[0].URL, "http://"),

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.

Looks really good!

Copy link
Contributor

@kohidave kohidave left a comment

Choose a reason for hiding this comment

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

Can you also take a look at this block? https://github.com/aws/amazon-ecs-cli-v2/pull/733/files#diff-3c6b0eccdf2aa38416345540f2b37b22R98

You'll have trim the 'http://' as well

@SoManyHs
Copy link
Contributor

Can you also take a look at this block? https://github.com/aws/amazon-ecs-cli-v2/pull/733/files#diff-3c6b0eccdf2aa38416345540f2b37b22R98

You'll have trim the 'http://' as well

which block? wasn't sure if you meant to include some line numbers in that link -- for me it takes me to line 100 of multi_app_project_test.go

@huanjani
Copy link
Contributor Author

huanjani commented Mar 13, 2020

This looks great!! 🎉 Could you also add output from manually testing the app show command plz? :)

Yup.
Before:

$ ecs-preview app show

? Which application of dinder would you like to show? webapp
About

  Project           dinder
  Name              webapp
  Type              Load Balanced Web App

Configurations

  Environment       Tasks               CPU (vCPU)          Memory (MiB)        Port
  test              1                   0.25                512                 3000

Routes

  Environment       URL                                                              Path
  test              dinde-Publi-EXUI6JTIP4L3-1856349916.us-west-2.elb.amazonaws.com  *

Variables

  Name                      Environment         Value
  ECS_CLI_APP_NAME          test                webapp
  ECS_CLI_ENVIRONMENT_NAME  test                test
  ECS_CLI_LB_DNS            test                dinde-Publi-EXUI6JTIP4L3-1856349916.us-west-2.elb.amazonaws.com
  ECS_CLI_PROJECT_NAME      test                dinder

After:

$ ecs-preview app show

? Which application of dinder would you like to show? webapp
About

  Project           dinder
  Name              webapp
  Type              Load Balanced Web App

Configurations

  Environment       Tasks               CPU (vCPU)          Memory (MiB)        Port
  test              1                   0.25                512                 3000

Routes

  Environment       URL
  test              http://dinde-Publi-EXUI6JTIP4L3-1856349916.us-west-2.elb.amazonaws.com/*

Variables

  Name                      Environment         Value
  ECS_CLI_APP_NAME          test                webapp
  ECS_CLI_ENVIRONMENT_NAME  test                test
  ECS_CLI_LB_DNS            test                dinde-Publi-EXUI6JTIP4L3-1856349916.us-west-2.elb.amazonaws.com
  ECS_CLI_PROJECT_NAME      test                dinder

Copy link
Contributor

@kohidave kohidave left a comment

Choose a reason for hiding this comment

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

Fresh ✨

@huanjani huanjani merged commit 23763a0 into aws:master Mar 14, 2020
@huanjani huanjani deleted the add-protocol branch March 14, 2020 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/svc Issues about services.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants