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

Use go for ecs-show-service-logs #1700

Merged
merged 1 commit into from Feb 13, 2019

Conversation

2 participants
@pjdufour-truss
Copy link
Contributor

pjdufour-truss commented Feb 4, 2019

Description

Uses a go program in place of the ecs-show-service-logs and ecs-show-service-stopped-logs scripts. This new program can handle both ECS ARN formats (old and new).

Reviewer Notes

I keep the bash scripts as wrappers for backwards compatibility but maybe we should just remove them?

Setup

Add any steps or code to run in this section to help others prepare to run your code:

go run cmd/ecs-show-service-logs/main.go --cluster app-staging --service app --limit 10

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using bin/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in IE.
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Screenshots

N.A.

@pjdufour-truss pjdufour-truss requested a review from chrisgilmerproj Feb 4, 2019


//fmt.Println(strings.Repeat("#", 30))
//fmt.Println(fmt.Sprintf("Task / Container: %s / %s ", *task.TaskArn, containerName))
//fmt.Println(strings.Repeat("-", 30))

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 6, 2019

Contributor

Are these for testing?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Feb 11, 2019

Author Contributor

will remove

@@ -0,0 +1,388 @@
package main

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 6, 2019

Contributor

Should we make this build in the Makefile? Or are we always going to use the go run version in the scripts?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Feb 11, 2019

Author Contributor

I'd rather just keep the bash script wrappers and not add to the makefile (at least for now).

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 11, 2019

Contributor

I thought about this some more and I think we should build it. The reason is that running make build_tools will also call the go_deps target which will run dep ensure -v. That means I'll get the aws-vault requirements installed for me. If we don't have it build in that manner then people will get a warning and may not know how to fix it immediately. Maybe we can call it bin/ecs-service-logs? And then the bash scripts can call that go command directly?

flag.String("service", "", "The service name")
flag.String("status", "", "The task status: "+strings.Join(ecsTaskStatuses, ", "))
flag.Int("limit", -1, "The log limit")
flag.Bool("verbose", false, "Print section lines")

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 6, 2019

Contributor

If you use flag.BoolP you can include a short flag name so we can have -v for this.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I've added a few comments. Let me know when you want me to review again. This looks pretty good and I'm excited to get this in.

@pjdufour-truss pjdufour-truss force-pushed the future_ecs_arn branch from 2194c34 to 03ad933 Feb 8, 2019

@pjdufour-truss pjdufour-truss changed the title WIP / Use go for ecs-show-service-logs Use go for ecs-show-service-logs Feb 11, 2019

@pjdufour-truss pjdufour-truss force-pushed the future_ecs_arn branch from 03ad933 to 7a89cba Feb 11, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #1700 into master will decrease coverage by 0.56%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1700      +/-   ##
=========================================
- Coverage   47.56%     47%   -0.57%     
=========================================
  Files         410     410              
  Lines       17276   17208      -68     
  Branches     1575    1593      +18     
=========================================
- Hits         8218    8089     -129     
- Misses       8331    8395      +64     
+ Partials      727     724       -3
Flag Coverage Δ
#go 56.6% <ø> (-0.48%) ⬇️
#javascript 32.53% <ø> (-0.37%) ⬇️
Impacted Files Coverage Δ
src/shared/Swagger/selectors.js 0% <0%> (-68%) ⬇️
src/scenes/Office/api.js 4.34% <0%> (-15.66%) ⬇️
src/shared/Entities/modules/orders.js 0% <0%> (-12.5%) ⬇️
src/scenes/Office/ducks.js 33.33% <0%> (-12.29%) ⬇️
pkg/models/validators.go 71.11% <0%> (-4.82%) ⬇️
pkg/dates/calculators.go 70.58% <0%> (-4.42%) ⬇️
pkg/models/shipment.go 51.02% <0%> (-3.83%) ⬇️
src/shared/Entities/modules/ppms.js 17.39% <0%> (-0.47%) ⬇️
src/shared/Entities/schema.js 100% <0%> (ø) ⬆️
src/scenes/Office/OrdersInfo.jsx 0% <0%> (ø) ⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d53bff8...621a945. Read the comment docs.

flag.String("environment", "", "The environment name")
flag.String("service", "", "The service name")
flag.String("status", "", "The task status: "+strings.Join(ecsTaskStatuses, ", "))
flag.Int("limit", -1, "The log limit")

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 11, 2019

Contributor

We should include some info, like 1 and above will return results limited by a number. Because they could put in 0 I guess.

aws logs get-log-events --log-group-name "$log_group_name" --log-stream-name "$log_stream_name" --query 'events[].message' | jq -r '.[]' || true
echo
done
go run cmd/ecs-show-service-logs/main.go --cluster "app-${2}" --service "${1}" --status "RUNNING" --verbose

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 11, 2019

Contributor

I'm wondering if we should eliminate running --verbose here. I know the old scripts did it but I'd love to promote your idea of having all these scripts return JSON readable output by default.

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Feb 12, 2019

Author Contributor

In my personal opinion, I'd rather maintain backwards compatibility at the moment.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 12, 2019

Contributor

that's fine:)

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I requested some changes inline.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

⭐️

@pjdufour-truss pjdufour-truss force-pushed the future_ecs_arn branch from 7749b8c to 621a945 Feb 13, 2019

@pjdufour-truss pjdufour-truss merged commit 26a1d2f into master Feb 13, 2019

18 of 19 checks passed

codecov/project 47% (-0.57%) compared to d53bff8
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing d53bff8...621a945
Details

@pjdufour-truss pjdufour-truss deleted the future_ecs_arn branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment