-
Notifications
You must be signed in to change notification settings - Fork 111
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
Add test for unsupported Go #490
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #490 +/- ##
==========================================
+ Coverage 77.14% 77.40% +0.26%
==========================================
Files 67 67
Lines 5302 5302
==========================================
+ Hits 4090 4104 +14
+ Misses 990 979 -11
+ Partials 222 219 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not that I have powers to actually aprove it but...) it looks good to me!
I left a single question about the autoinstrumenter
config filename, but that's definitely not a blocker.
dockerfile: ./test/integration/components/beyla/Dockerfile | ||
command: | ||
- /beyla | ||
- --config=/configs/instrumenter-config-java.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be the Java one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's fine :). The java one doesn't have jaeger in, so I just stole one that existed to avoid making yet another config file.
Hi! Thanks for adding this. I don't get the tests. Shouldn't we just check that Beyla couldn't instrument a given process because it's Go 1.16? |
Yes, this is going to be done through this PR #485. I added the test, but with skipping Go tracers, so that we can use it in that PR. |
Adds a Go 1.16 basic integration test, which only works because we have:
in the config