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

feat: container ID detector for cgroup v2 #1181

Merged
merged 26 commits into from
Oct 1, 2022
Merged

feat: container ID detector for cgroup v2 #1181

merged 26 commits into from
Oct 1, 2022

Conversation

abhee11
Copy link
Contributor

@abhee11 abhee11 commented Sep 19, 2022

Which problem is this PR solving?

#1173
docker v2 detector

Short description of the changes

  • generalized detector for docker v1, v2 and containerd

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

@abhee11 abhee11 requested a review from a team September 19, 2022 02:05
@abhee11 abhee11 changed the title docker v2 detector feat: docker v2 detector Sep 19, 2022
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #1181 (2c4d709) into main (60a92c7) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
- Coverage   96.08%   96.05%   -0.03%     
==========================================
  Files          14       16       +2     
  Lines         893      937      +44     
  Branches      191      198       +7     
==========================================
+ Hits          858      900      +42     
- Misses         35       37       +2     
Impacted Files Coverage Δ
...ector-container/src/detectors/ContainerDetector.ts 95.34% <100.00%> (ø)
...resource-detector-container/src/detectors/index.ts 100.00% <100.00%> (ø)

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks
All open threads are optional. Feel free to resolve or implement

@abhee11
Copy link
Contributor Author

abhee11 commented Sep 22, 2022

@blumamir I addressed the rest of the comments as well - please merge when you get a chance.

Thank you so much for the review.

@rauno56 rauno56 changed the title feat: docker v2 detector feat: container ID detector for cgroup v2 Sep 22, 2022
@rauno56
Copy link
Member

rauno56 commented Sep 30, 2022

Thanks for working on this. Please add tests with files having multiple lines and we'll be golden!

@abhee11
Copy link
Contributor Author

abhee11 commented Sep 30, 2022

@rauno56 Changed existing test to take multiple lines as input - I think we should be covered - thank you for the review - feel free to merge when you can.

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with the long back-and-forth! I don't have any blockers.
Only curious about the length check.

@abhee11
Copy link
Contributor Author

abhee11 commented Sep 30, 2022

@rauno56 I strongly believe in code reviews like these ones, may be a bit long but it's a good learning and quality check. Thank you for an engaged review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants