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

Merged
merged 4 commits into from
Aug 22, 2024

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 96.49123% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.80%. Comparing base (959e7bc) to head (ad68568).
Report is 4 commits behind head on main.

Files Patch % Lines
cmd/builder/internal/builder/main.go 95.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10098      +/-   ##
==========================================
+ Coverage   91.62%   91.80%   +0.18%     
==========================================
  Files         406      406              
  Lines       19046    19080      +34     
==========================================
+ Hits        17450    17516      +66     
+ Misses       1237     1216      -21     
+ Partials      359      348      -11     

☔ 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
cmd/builder/internal/builder/main.go Outdated Show resolved Hide resolved
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

@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.

Copy link
Contributor

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

@mx-psi
Copy link
Member

mx-psi commented Jun 13, 2024

@kristinapathak feel free to ping me when tests are passing :)

@kristinapathak kristinapathak force-pushed the builder-skip-go-mod branch 4 times, most recently from 55cb39f to a3cbda6 Compare June 27, 2024 21:38
@@ -144,7 +155,7 @@ func GetModules(cfg Config) error {
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

go mod tidy is run a few lines above and downloads the needed dependencies, so go mod download isn't needed. No dependencies are updated after the tidy call.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, this comment was useful!

@kristinapathak kristinapathak force-pushed the builder-skip-go-mod branch 3 times, most recently from 9a37a4c to 893f878 Compare June 28, 2024 02:42
updatespec := "-require=" + mod + "@" + ver

if _, err := runGoCommand(*c, "mod", "edit", updatespec); err != nil {
return err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out how to get go mod edit to fail 😞 everything I tried caused other failures...

@kristinapathak kristinapathak marked this pull request as ready for review June 28, 2024 03:06
@kristinapathak kristinapathak requested review from a team and atoulme June 28, 2024 03:06
@kristinapathak kristinapathak force-pushed the builder-skip-go-mod branch 2 times, most recently from 5de8cc9 to ac8a09b Compare July 8, 2024 19:44
@mx-psi
Copy link
Member

mx-psi commented Jul 18, 2024

CI failures are due to #10661, will merge this PR after merging that one

@mx-psi
Copy link
Member

mx-psi commented Jul 18, 2024

--- FAIL: TestGetModules (36.26s)
    --- FAIL: TestGetModules/Exporter_Not_in_Gomod (3.57s)
        main_test.go:633: 
            	Error Trace:	D:/a/opentelemetry-collector/opentelemetry-collector/cmd/builder/internal/builder/main_test.go:633
            	Error:      	Received unexpected error:
            	            	failed to compile the OpenTelemetry Collector distribution: go subcommand failed with args '[build -trimpath -o  -ldflags=-s -w]': exit status 1, error message: # go.opentelemetry.io/collector/config/configgrpc
Error:             	            	C:\Users\runneradmin\go\pkg\mod\go.opentelemetry.io\collector\config\[email protected]\configgrpc.go:274:62: not enough arguments in call to gcs.Auth.GetClientAuthenticator
            	            		have (map[component.ID]component.Component)
            	            		want (context.Context, map[component.ID]component.Component)

@kristinapathak
Copy link
Contributor Author

@mx-psi, can this be merged?

@mx-psi
Copy link
Member

mx-psi commented Jul 31, 2024

@kristinapathak Sorry, should have said something explicitly, CI is not passing so I can't merge (it's a required CI so I literally can't click the button 😄). This error is still happening #10098 (comment)

@mx-psi
Copy link
Member

mx-psi commented Aug 6, 2024

Looks like CI is still having trouble

@kristinapathak
Copy link
Contributor Author

@mx-psi, tests are back to passing 🙂

@mx-psi
Copy link
Member

mx-psi commented Aug 13, 2024

Looks like there is one last test that is timing out on Windows:

panic: test timed out after 4m0s
running tests:
	TestGetModules (17s)
	TestGetModules/Exporter_Not_in_Gomod (3s)

I am fine with increasing the timeout value. Could you go ahead and do that?

@mx-psi
Copy link
Member

mx-psi commented Aug 14, 2024

Looks like bumping the timeout revealed an error:

	Error:      	Error "component dependency not found in go mod file: 'go.opentelemetry.io/collector/confmap/provider/envprovider'. Use --skip-strict-versioning to temporarily disable this check. This flag will be removed in a future minor version" does not contain "failed to generate source file"

@mx-psi
Copy link
Member

mx-psi commented Aug 16, 2024

🤔

Another error

--- FAIL: TestGenerateAndCompile (74.27s)
    --- FAIL: TestGenerateAndCompile/No_Dir_Permissions (2.51s)
        main_test.go:441: 
            	Error Trace:	D:/a/opentelemetry-collector/opentelemetry-collector/cmd/builder/internal/builder/main_test.go:441
            	Error:      	An error is expected but got nil.
            	Test:       	TestGenerateAndCompile/No_Dir_Permissions

@kristinapathak kristinapathak force-pushed the builder-skip-go-mod branch 2 times, most recently from 15b11a2 to e888fd3 Compare August 21, 2024 17:08
@mx-psi mx-psi merged commit a0331ed into open-telemetry:main Aug 22, 2024
49 checks passed
@mx-psi
Copy link
Member

mx-psi commented Aug 22, 2024

🎉 🎉

@github-actions github-actions bot added this to the next release milestone Aug 22, 2024
@kristinapathak kristinapathak deleted the builder-skip-go-mod branch August 22, 2024 17:27
codeboten added a commit to codeboten/opentelemetry-collector that referenced this pull request Aug 27, 2024
codeboten added a commit that referenced this pull request Aug 27, 2024
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.

2 participants