Skip to content

Conversation

@txabman42
Copy link
Contributor

Contributing to #196

Support building non-root paths e.g. otel go build -a ./app/vmctl

@txabman42 txabman42 requested a review from a team as a code owner November 27, 2025 20:12
@github-actions github-actions bot added the scope:feat A new feature being added label Nov 27, 2025
@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

❌ Patch coverage is 60.78431% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.32%. Comparing base (3bd8623) to head (00d7c0d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
tool/internal/setup/setup.go 53.48% 18 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #209      +/-   ##
==========================================
+ Coverage   51.22%   53.32%   +2.10%     
==========================================
  Files          48       48              
  Lines        3274     3306      +32     
==========================================
+ Hits         1677     1763      +86     
+ Misses       1477     1420      -57     
- Partials      120      123       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@txabman42 txabman42 force-pushed the support-non-roout-build-path branch from 4a58638 to 22e237d Compare November 27, 2025 20:21
@txabman42
Copy link
Contributor Author

Not sure that we want to have code coverage as required. I usually don't like to require that, even less if right now great part of our app is tested via integration/e2e tests.

@txabman42 txabman42 force-pushed the support-non-roout-build-path branch from 63cdc08 to 3bfc313 Compare November 28, 2025 08:22
@txabman42 txabman42 requested a review from y1yang0 December 3, 2025 07:38
@kakkoyun
Copy link
Member

kakkoyun commented Dec 3, 2025

@y1yang0 Could you have another look at this?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds support for building non-root package paths (e.g., otel go build -a ./app/vmctl) by implementing package discovery functionality. The implementation uses the golang.org/x/tools/go/packages library to correctly identify and process build targets from command-line arguments.

Key Changes

  • Package Discovery: New GetBuildPackages function parses go build arguments to identify target packages, handling flags, multiple packages, and wildcard patterns
  • Multi-package Support: The setup phase now generates otel.runtime.go files in each target package directory rather than only in the root
  • Deterministic Imports: Import declarations in generated files are now sorted for consistent output

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
tool/util/go.go Adds GetBuildPackages function to extract package patterns from build arguments and load corresponding packages
tool/util/go_test.go Comprehensive tests for package discovery covering single/multiple packages, wildcards, flags, and edge cases
tool/internal/setup/setup.go Integrates package discovery to generate instrumentation files in each target package directory and update cleanup logic
tool/internal/setup/add.go Updates addDeps to accept package path parameter, adds deterministic import ordering, and improves logging
tool/internal/setup/add_test.go Golden file tests for the addDeps function covering various rule configurations
tool/internal/setup/testdata/*.golden Golden files for testing generated otel.runtime.go content
go.mod Moves golang.org/x/tools from indirect to direct dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@txabman42 txabman42 force-pushed the support-non-roout-build-path branch from 688e5dc to 08c44c3 Compare December 3, 2025 16:02
@y1yang0
Copy link
Contributor

y1yang0 commented Dec 4, 2025

I'll take a look tomorrow. Thanks

Copy link
Contributor

@y1yang0 y1yang0 left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@txabman42 txabman42 force-pushed the support-non-roout-build-path branch from 143ee4f to 37206ef Compare December 8, 2025 10:27
@txabman42 txabman42 force-pushed the support-non-roout-build-path branch from 37206ef to 458fb2f Compare December 8, 2025 10:49
@txabman42 txabman42 force-pushed the support-non-roout-build-path branch from 458fb2f to 00d7c0d Compare December 8, 2025 10:49
@kakkoyun kakkoyun merged commit fb6c330 into open-telemetry:main Dec 8, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope:feat A new feature being added

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants