[builder] Add strict versioning #9897

kristinapathak · 2024-04-05T03:50:41Z

Description:
Adds strict version checking in the builder. This enables users to ensure that the versions specified in the builder config are the versions used in the go.mod when building the collector binary. This can be disabled with --skip-strict-versioning.

Link to tracking Issue: #9896

Testing: Added unit tests

Documentation: Added to builder README

codecov · 2024-04-05T03:59:04Z

Codecov Report

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

Project coverage is 91.76%. Comparing base (f1a7475) to head (d821f83).
Report is 31 commits behind head on main.

Files	Patch %	Lines
cmd/builder/internal/builder/main.go	74.28%	11 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9897      +/-   ##
==========================================
- Coverage   91.93%   91.76%   -0.18%     
==========================================
  Files         357      358       +1     
  Lines       16501    16604     +103     
==========================================
+ Hits        15171    15236      +65     
- Misses       1001     1041      +40     
+ Partials      329      327       -2

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

mx-psi

Thanks for filing this separately. I think this should be the default behavior, we have had people confused about this in the past (see #8692) and I think it is better to spit out an error than to silently do the wrong thing.

I would therefore prefer to make this PR into a breaking change and add a temporary flag (--skip-strict-versioning?) to temporarily allow users to revert this check and allow them to migrate. We can mention this flag in the error message and remove it after a few versions.

Would you mind switching from --strict-versioning to --skip-strict-versioning and updating the release note?

Thanks!

cc @open-telemetry/collector-approvers for visibility

mx-psi

Thanks for the change! I left a few comments :)

cmd/builder/internal/builder/main.go

.chloggen/builder-strict-versioning.yaml

cmd/builder/README.md

cmd/builder/internal/builder/modfile/modfile.go

cmd/builder/internal/builder/config.go

cmd/builder/internal/builder/main.go

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>

- add back hardcoding retries - update strict mode error wording - use modfile pkg - update some config versions - consolidate runGoCommand funcs - swap function order, update comment

kristinapathak · 2024-04-11T22:03:36Z

@mx-psi, thanks for the review! I think I addressed everything. 🙂 Let me know what you think.

mx-psi

Code LGTM, I left a couple small suggestions to improve the error messages. Would love to increase the code coverage a bit. If that's too hard on this PR, just let me know, we can postpone to your second PR :)

mx-psi · 2024-04-12T09:10:15Z

cmd/builder/internal/builder/main.go

+	}
+
+	corePath, coreVersion := cfg.coreModuleAndVersion()
+	if dependencyVersions[corePath] != coreVersion {


The error here and below would be a bit confusing if corePath is not on dependencyVersions. While this would be a weird situation that I don't think we can encounter now, IMO for future-proofing we should do the full check (corePathVersion, ok := dependencyVersions[corePath]) and provide a more appropriate error message if ok is false

mx-psi · 2024-04-12T09:10:45Z

cmd/builder/internal/builder/main.go

+			continue
+		}
+
+		if dependencyVersions[module] != version {


Ditto here, IMO we should do the explicit check for presence in dependencyVersions

cmd/builder/internal/builder/main.go

kristinapathak · 2024-04-17T03:25:23Z

cmd/builder/internal/builder/main.go

+		if !ok {
+			return fmt.Errorf("component %w: '%s'. %s", ErrDepNotFound, module, skipStrictMsg)
+		}
+		if moduleDepVersion != version {


It'll be easier to add a test for this in #9631, so I'd prefer to wait and add it then.

kristinapathak · 2024-04-17T03:25:45Z

cmd/builder/internal/builder/main.go

+	if !ok {
+		return fmt.Errorf("core collector %w: '%s'. %s", ErrDepNotFound, corePath, skipStrictMsg)
+	}
+	if coreDepVersion != coreVersion {


It'll be easier to add a test for this in #9631, so I'd prefer to wait and add it then.

cmd/builder/internal/builder/main.go

kristinapathak · 2024-04-17T03:43:26Z

cmd/builder/internal/builder/main.go

 	cfg.Logger.Info("Getting go modules")
 	// basic retry if error from go mod command (in case of transient network error). This could be improved
 	// retry 3 times with 5 second spacing interval
 	retries := 3
 	failReason := "unknown"
 	for i := 1; i <= retries; i++ {
-		if err := runGoCommand(cfg, "mod", "download"); err != nil {
+		if _, err := runGoCommand(cfg, "mod", "download"); err != nil {


I haven't been able to add coverage for this because if I would expect this to fail, the previous go get command fails first.

kristinapathak · 2024-04-17T03:44:12Z

cmd/builder/internal/builder/main.go

+					c.Connectors...)...)...)...)
+}
+
+func (c *Config) readGoModFile() (string, map[string]string, error) {


I haven't been able to add coverage for this function because in the code path, if I would expect this to fail, the previous go get command fails first.

I can add a separate test that targets this function directly, otherwise not sure how to test this.

kristinapathak · 2024-04-17T03:44:39Z

cmd/builder/internal/builder/main.go

+		}
+
+		moduleDepVersion, ok := dependencyVersions[module]
+		if !ok {


It'll be easier to add a test for this in #9631, so I'd prefer to wait and add it then.

kristinapathak · 2024-04-17T03:51:00Z

@mx-psi, let me know your thoughts on my unit test comments. 🙂

mx-psi

Thanks for taking a thorough look at coverage. We can address the remaining easy cases in the next PR.

This LGTM, but in order to merge, I need you to fix the linter error:

 internal/builder/main.go:131:49: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
		return fmt.Errorf("%w: %v", errCompileFailed, err)

#### Description Improves `TestVersioning` added on #9897. The configurations were not what one would usually find, since when running the builder we do some validation and setup. This makes it closer to the real case. In the process I removed the 'invalid' cases, since these error out today already

Partially reverts #9897. This was not documented on the original PR and is IMO too strict. We likely want to allow for some skew between versions. Mentioned on #9513 (comment) --------- Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>

jmacd approved these changes Apr 5, 2024

View reviewed changes

mx-psi reviewed Apr 9, 2024

View reviewed changes

kristinapathak force-pushed the builder-strict-versioning branch from 9f4c731 to 102319b Compare April 10, 2024 04:03

kristinapathak changed the title ~~[builder] Add --strict-versioning flag~~ [builder] Add strict versioning Apr 10, 2024

kristinapathak marked this pull request as ready for review April 10, 2024 04:14

kristinapathak requested review from a team and Aneurysm9 April 10, 2024 04:14

mx-psi requested changes Apr 10, 2024

View reviewed changes

kristinapathak force-pushed the builder-strict-versioning branch from 8e501c2 to 5c43c8a Compare April 10, 2024 21:20

[builder] Add --strict-versioning flag

b302a23

kristinapathak force-pushed the builder-strict-versioning branch from cbda7d5 to 49d2832 Compare April 11, 2024 19:35

Kristina Pathak and others added 3 commits April 11, 2024 14:59

make strict versioning the default, add a few tests

c40267d

add suggested wording that flag is temporary

fe9b80c

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>

lots of small fixes

90b000f

- add back hardcoding retries - update strict mode error wording - use modfile pkg - update some config versions - consolidate runGoCommand funcs - swap function order, update comment

kristinapathak force-pushed the builder-strict-versioning branch from 4013dda to 90b000f Compare April 11, 2024 22:01

kristinapathak requested a review from mx-psi April 11, 2024 22:03

mx-psi reviewed Apr 12, 2024

View reviewed changes

add explicit errors when an expected dependency isn't found in the gomod

a2fdb95

kristinapathak commented Apr 17, 2024

View reviewed changes

mx-psi approved these changes Apr 17, 2024

View reviewed changes

add tests

d821f83

kristinapathak force-pushed the builder-strict-versioning branch from c0f792f to d821f83 Compare April 17, 2024 20:54

mx-psi merged commit e9b432d into open-telemetry:main Apr 18, 2024
48 of 49 checks passed

github-actions bot added this to the next release milestone Apr 18, 2024

kristinapathak deleted the builder-strict-versioning branch April 18, 2024 16:42

evan-bradley mentioned this pull request Apr 19, 2024

[cmd/builder] Allow configuring confmap providers #9513

Merged

mx-psi mentioned this pull request Apr 19, 2024

[cmd/builder] Remove undocumented strictness check #9999

Merged

mx-psi mentioned this pull request Apr 19, 2024

[cmd/builder] Improve TestVersioning #10000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[builder] Add strict versioning #9897

[builder] Add strict versioning #9897

kristinapathak commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

mx-psi left a comment

mx-psi left a comment

kristinapathak commented Apr 11, 2024

mx-psi left a comment

mx-psi Apr 12, 2024

mx-psi Apr 12, 2024

kristinapathak Apr 17, 2024

kristinapathak Apr 17, 2024

kristinapathak Apr 17, 2024

kristinapathak Apr 17, 2024

kristinapathak Apr 17, 2024

kristinapathak commented Apr 17, 2024

mx-psi left a comment

[builder] Add strict versioning #9897

[builder] Add strict versioning #9897

Conversation

kristinapathak commented Apr 5, 2024 • edited Loading

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

mx-psi left a comment

Choose a reason for hiding this comment

mx-psi left a comment

Choose a reason for hiding this comment

kristinapathak commented Apr 11, 2024

mx-psi left a comment

Choose a reason for hiding this comment

mx-psi Apr 12, 2024

Choose a reason for hiding this comment

mx-psi Apr 12, 2024

Choose a reason for hiding this comment

kristinapathak Apr 17, 2024

Choose a reason for hiding this comment

kristinapathak Apr 17, 2024

Choose a reason for hiding this comment

kristinapathak Apr 17, 2024

Choose a reason for hiding this comment

kristinapathak Apr 17, 2024

Choose a reason for hiding this comment

kristinapathak Apr 17, 2024

Choose a reason for hiding this comment

kristinapathak commented Apr 17, 2024

mx-psi left a comment

Choose a reason for hiding this comment

kristinapathak commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading