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

Adding code to address docker healthcheck issue #1285

Merged
merged 24 commits into from
Oct 26, 2022
Merged

Adding code to address docker healthcheck issue #1285

merged 24 commits into from
Oct 26, 2022

Conversation

PaurushGarg
Copy link
Member

@PaurushGarg PaurushGarg commented Jun 7, 2022 •
edited by rapphil
Loading

Description:

  1. Fixing issue Fargate/ECS healthcheck #1124 related to ECS healthcheck

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@PaurushGarg PaurushGarg requested a review from a team as a code owner June 7, 2022 18:51
@bryan-aguilar
Copy link
Contributor

You're going to want to ensure that the built health check tool is added to the .gitignore.

@bryan-aguilar bryan-aguilar marked this pull request as draft June 7, 2022 18:56
bryan-aguilar
bryan-aguilar reviewed Jun 7, 2022
View reviewed changes
Copy link
Contributor

@bryan-aguilar bryan-aguilar left a comment

Choose a reason for hiding this comment

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

Also, don't forget about documentation for this. Where will it go? How can this new health check tool be used? Look at the original issue for motivation.

cmd/awscollector/Dockerfile Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
bryan-aguilar
bryan-aguilar reviewed Jun 23, 2022
View reviewed changes
Copy link
Contributor

@bryan-aguilar bryan-aguilar left a comment

Choose a reason for hiding this comment

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

Please add health_check to the .gitignore.

Makefile Outdated Show resolved Hide resolved
@PaurushGarg
Copy link
Member Author

Please add health_check to the .gitignore.

Hi Bryan, currently health_check binary path is inside build/ directory, and this directory is already part of .gitignore.

@bryan-aguilar
Copy link
Contributor

bryan-aguilar commented Jun 24, 2022 •
edited
Loading

Please add health_check to the .gitignore.

Hi Bryan, currently health_check binary path is inside build/ directory, and this directory is already part of .gitignore.

The binary file health_check in the root directory was committed as part of this PR and should be added to the .gitignore or removed from the tracked files.

@PaurushGarg PaurushGarg changed the title Adding draft code to address docker healthcheck issue [Draft/WIP] Adding code to address docker healthcheck issue Jul 30, 2022
@jeromeinsf jeromeinsf removed their request for review August 22, 2022 02:32
@PaurushGarg PaurushGarg requested a review from a team August 23, 2022 18:54
@PaurushGarg PaurushGarg marked this pull request as ready for review August 29, 2022 23:43
This was referenced Sep 2, 2022
Merged
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2022

This PR is stale because it has been open 30 days with no activity.

@github-actions github-actions bot added the stale label Oct 9, 2022
@PaurushGarg PaurushGarg removed the stale label Oct 13, 2022
Aneurysm9
Aneurysm9 reviewed Oct 13, 2022
View reviewed changes
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
bryan-aguilar
bryan-aguilar reviewed Oct 20, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
docs/developers/docker-demo.md Outdated Show resolved Hide resolved
@bryan-aguilar
Copy link
Contributor

Can we add unit tests for the validatePort function? A set of straightforward assertions should do.

rapphil
rapphil reviewed Oct 20, 2022
View reviewed changes
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
cmd/healthcheck/main.go Outdated Show resolved Hide resolved
deployment-template/ecs/aws-otel-ec2-sidecar-deployment-cfn.yaml Outdated Show resolved Hide resolved
config/ecs/ecs-default-config.yaml Outdated Show resolved Hide resolved
config/ecs/container-insights/otel-task-metrics-config.yaml Outdated Show resolved Hide resolved
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
@PaurushGarg
Copy link
Member Author

PaurushGarg commented Oct 24, 2022 •
edited
Loading

Can we add unit tests for the validatePort function? A set of straightforward assertions should do.

Thanks. Added.

@PaurushGarg
Removing host and path from command line arguments and updating make …
f26b71c 
…file for build in more architecture
@PaurushGarg
Deleting binary file and adding test cases
321da87
@PaurushGarg
Removing lint errors
50cc518
@PaurushGarg
Roll back changes to makefile to build healthcheck for multiple archi…
c89eff0 
…tectures
@PaurushGarg
Removing changes from dockerfile as they will only part of execution …
240e19c 
…instructions
@PaurushGarg
Adding changes to build multi-platform
2f36a68
@PaurushGarg
Removing healthcheck from dockerfile
6856c9c
@PaurushGarg
Adding code changes to config and template files for healthcheck setu…
e35fe1c 
…p instructions
@PaurushGarg
Resolving comments
9dd4611
@PaurushGarg
Updating default path to match current default path of opentelemetry …
94e18a1 
…health extension
@PaurushGarg
Resolving comments
aa04ba5
@PaurushGarg
Reverting the changes to ec2-sidecar-deployment-cfn
37888a4
@PaurushGarg
Reverting unintended change to config.yaml
f1722b2
bryan-aguilar
bryan-aguilar reviewed Oct 25, 2022
View reviewed changes
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
cmd/healthcheck/main_test.go Outdated Show resolved Hide resolved
bryan-aguilar
bryan-aguilar suggested changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@bryan-aguilar bryan-aguilar left a comment

Choose a reason for hiding this comment

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

Please ensure that a PR is submitted and merged to add the ecshealthcheck test case to testcases.json file before this PR is merged.

@bryan-aguilar bryan-aguilar merged commit b19bd23 into aws-observability:main Oct 26, 2022
@sjparkinson sjparkinson mentioned this pull request Jan 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryan-aguilar bryan-aguilar bryan-aguilar approved these changes

@rapphil rapphil rapphil approved these changes

@erichsueh3 erichsueh3 Awaiting requested review from erichsueh3

@vasireddy99 vasireddy99 Awaiting requested review from vasireddy99

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

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

Successfully merging this pull request may close these issues.
4 participants
@PaurushGarg @bryan-aguilar @Aneurysm9 @rapphil