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

[builder] Support for --skip-new-go-module #10098

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kristinapathak
Copy link
Contributor

A continuation of #9253 and #9631

Description: Adds a --skip-new-go-module flag to the OTC builder. This enables users working in an existing go module environment (say, a "monorepo") to update the module they have, vs forcing the use of a new module.

With the new support inside an existing Go module, a collector main package can be generated using a go:generate directive. For example, in the directory where I want my collector built, the file generate.go has this line:

//go:generate builder --skip-new-go-module --skip-compilation --strict-versioning --config=./build-config.yaml
In the same directory, the build-config.yaml describes the collector to build. The builder generates the other files in the same directory. At this point, normal Go workflows can be used to update indirect dependencies.

Link to tracking Issue: #9252

Testing: Will add unit tests in the next few days.

Documentation: Yes.

Copy link

linux-foundation-easycla bot commented May 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 91.49%. Comparing base (d294537) to head (3b91ace).
Report is 12 commits behind head on main.

Current head 3b91ace differs from pull request most recent head b901f03

Please upload reports for the commit b901f03 to get more accurate results.

Files Patch % Lines
cmd/builder/internal/builder/main.go 18.42% 30 Missing and 1 partial ⚠️
cmd/builder/internal/command.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10098      +/-   ##
==========================================
- Coverage   91.90%   91.49%   -0.41%     
==========================================
  Files         361      362       +1     
  Lines       16970    16799     -171     
==========================================
- Hits        15596    15371     -225     
- Misses       1032     1087      +55     
+ Partials      342      341       -1     

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

cmd/builder/internal/builder/config.go Outdated Show resolved Hide resolved
cmd/builder/README.md Outdated Show resolved Hide resolved
cmd/builder/README.md Outdated Show resolved Hide resolved
cmd/builder/internal/builder/main.go Outdated Show resolved Hide resolved
// upgrading or changing version
updatespec := mod + "@" + ver

if _, err := runGoCommand(*c, "get", updatespec); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure which one would be better, but we also have the option of doing go mod edit -require=example.org@v1.2.0. Why is go get preferred here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell, the difference is that go get installs and builds the packages needed whereas go mod edit doesn't. Given that we run go mod tidy after this in GetModules(), I'll change this to use go mod edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing to note about using go mod edit -require:

These flags are mainly for tools that understand the module graph. Users should prefer go get path@version or go get path@none, which make other go.mod adjustments as needed to satisfy constraints imposed by other modules.

I'm not sure what these other adjustments are or if we need them here? I would expect if we change the go.mod into some incompatible graph that go mod tidy will fail after this.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about this, I think I would prefer using go mod edit -require at first. The section you quoted sounds to me as "go get also does go mod tidy, which you probably want", and since, as you point out, we already go mod tidy after this, I would prefer to keep things simple

cmd/builder/internal/command.go Outdated Show resolved Hide resolved
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 24, 2024
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Is there any chance to increase testing coverage here? Some error cases would be harder to reach, but there are other parts that should be doable

@github-actions github-actions bot removed the Stale label May 25, 2024
@kristinapathak
Copy link
Contributor Author

@mx-psi, sorry for the delay. I hit this issue when adding back tests and then got pulled into other things:

failed to update go.mod: go subcommand failed with args '[mod tidy -compat=1.21]': exit status 1, error message: go: finding module for package go.opentelemetry.io/collector/internal/testdata
        	            	go: finding module for package go.opentelemetry.io/collector/confmap/converter/expandconverter
        	            	go: found go.opentelemetry.io/collector/confmap/converter/expandconverter in go.opentelemetry.io/collector/confmap/converter/expandconverter v0.101.0
        	            	go: finding module for package go.opentelemetry.io/collector/internal/testdata
        	            	go: go.opentelemetry.io/collector/cmd/builder/internal/tester/output imports
        	            		go.opentelemetry.io/collector/connector tested by
        	            		go.opentelemetry.io/collector/connector.test imports
        	            		go.opentelemetry.io/collector/internal/testdata: module go.opentelemetry.io/collector@latest found (v0.101.0), but does not contain package go.opentelemetry.io/collector/internal/testdata

I'll try to fix this and increase coverage more this week.

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.

None yet

2 participants