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

Container insights ssl #822

Merged
merged 16 commits into from
Oct 16, 2023
Merged

Container insights ssl #822

merged 16 commits into from
Oct 16, 2023

Conversation

bhanuba
Copy link
Contributor

@bhanuba bhanuba commented Aug 23, 2023

Description of the issue

In ccwa emf uses an otel exporter instead of a cwa exporter. We need to test this still works with ca bundle or not.

Description of changes

Here we are testing the container insights to check whether it works with the ca_bundle or not.

  • Automated container insights test using ca bundle.
  • Fix the ca bundle issue with CCWA in container insights.

License

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

Tests

Unit tests passed

Requirements

Before commit the code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

@bhanuba bhanuba requested a review from a team as a code owner August 23, 2023 18:07
translator/tocwconfig/tocwconfig_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@sky333999
Copy link
Contributor

Mind updating the title and the description of the PR to be provide some more details. sll seems like a typo. And Im not very sure what Fixed Ca_Bundle issue is.

Also, how did we verify that this fix works?

@bhanuba bhanuba requested a review from SaxyPandaBear August 24, 2023 18:21
@SaxyPandaBear
Copy link
Contributor

Firstly, can you please update the PR title? It doesn't properly convey what this code change is for.

The PR looks good but I think it's lacking some end-to-end testing. Unit testing isn't enough here. Can you build the agent with this code change and validate it yourself?

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (96d4763) 57.58% compared to head (60f1f3a) 62.43%.
Report is 413 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
+ Coverage   57.58%   62.43%   +4.85%     
==========================================
  Files         370      339      -31     
  Lines       17548    17110     -438     
==========================================
+ Hits        10105    10683     +578     
+ Misses       6848     5873     -975     
+ Partials      595      554      -41     
Files Coverage Δ
cfg/commonconfig/commonconfig.go 8.00% <ø> (ø)
...md/amazon-cloudwatch-agent-config-wizard/wizard.go 59.55% <ø> (-8.51%) ⬇️
...amazon-cloudwatch-agent/amazon-cloudwatch-agent.go 2.68% <ø> (ø)
...oudwatch-agent/register_event_logger_notwindows.go 0.00% <ø> (ø)
...-cloudwatch-agent/register_event_logger_windows.go 0.00% <ø> (ø)
cmd/config-translator/translator.go 0.00% <ø> (ø)
cmd/xray-migration/commands_unix.go 42.50% <ø> (ø)
cmd/xray-migration/commands_windows.go 42.50% <ø> (ø)
cmd/xray-migration/xray-migration.go 30.28% <ø> (ø)
handlers/agentinfo/info.go 84.94% <ø> (ø)
... and 22 more

... and 206 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SaxyPandaBear
Copy link
Contributor

There are merge conflicts that you should resolve in your PR before it can be merged

Copy link
Contributor

@SaxyPandaBear SaxyPandaBear 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 some evidence of manual testing to verify that this change works

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

This PR was marked stale due to lack of activity.

@github-actions github-actions bot added the Stale label Sep 6, 2023
@sethAmazon sethAmazon changed the title Container insights sll Container insights ssl Sep 26, 2023
sethAmazon
sethAmazon previously approved these changes Sep 26, 2023
@sethAmazon
Copy link
Contributor

We ran integration tests that are failing in some os and passing in others. The failures are for reasons apart from SSL connection.

@github-actions github-actions bot removed the Stale label Sep 27, 2023
amazon-cloudwatch-agent Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity.

@github-actions github-actions bot added the Stale label Oct 11, 2023
@bhanuba bhanuba merged commit ef67cac into aws:main Oct 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants