Skip to content

Conversation

@kristinapathak
Copy link
Contributor

@kristinapathak kristinapathak commented Jun 27, 2024

Description

improving an error message - a missing gomod field would be reported but without informing where in the config the field is missing.

Link to tracking issue

Fixes #10474

Testing

Unit tests pass, added some tests for missing cases, manually tested

new error looks like:

../../bin/ocb_darwin_amd64 --config=./default.yaml
2024-06-27T11:39:10.316-0700	INFO	internal/command.go:125	OpenTelemetry Collector Builder	{"version": "", "date": "unknown"}
2024-06-27T11:39:10.319-0700	INFO	internal/command.go:161	Using config file	{"path": "./default.yaml"}
Error: invalid configuration: receiver module at index 0: missing gomod specification for module; provider module at index 2: missing gomod specification for module

@kristinapathak kristinapathak requested review from a team and songy23 June 27, 2024 18:28
@codecov
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (4a11a3e) to head (ea4a38a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10475   +/-   ##
=======================================
  Coverage   92.33%   92.33%           
=======================================
  Files         393      393           
  Lines       18647    18647           
=======================================
  Hits        17218    17218           
  Misses       1069     1069           
  Partials      360      360           

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

assert.True(t, strings.HasPrefix(cfg.Extensions[0].Name, "otlpreceiver"))
}

func TestInvalidModule(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add a test with an invalid but existing go.mod file?

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 can but that doesn't seem relevant to this PR? I'm wondering if this was confused for https://github.com/open-telemetry/opentelemetry-collector/pull/10098/files

Copy link
Member

Choose a reason for hiding this comment

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

Before the error said invalid gomod specification for module, so what I am wondering is if we are introducing a case where we say the go.mod file does not exist but what happens is that it's invalid. I am fine not adding the test on this PR if we are certain this does not happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only validation being done currently is if the GoMod value is empty: if mod.GoMod == ""; here:
https://github.com/open-telemetry/opentelemetry-collector/pull/10475/files#diff-5aa5d2605e296c63681940c00cbc83eb2bb6b2e67ad2071f5c5c6435537d5561R240
There is no other validation of this config field. The field isn't a go.mod file, and it normally looks something like: go.opentelemetry.io/collector/confmap/provider/envprovider v0.103.0

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for clearing that up, that message sure was confusing to me 😄 we can merge this

@kristinapathak kristinapathak force-pushed the builder-empty-gomod-field branch from f1a84b4 to ea4a38a Compare July 8, 2024 18:20
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.

Thanks, looks good to me!

assert.True(t, strings.HasPrefix(cfg.Extensions[0].Name, "otlpreceiver"))
}

func TestInvalidModule(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for clearing that up, that message sure was confusing to me 😄 we can merge this

@mx-psi mx-psi merged commit 8de544b into open-telemetry:main Jul 8, 2024
@github-actions github-actions bot added this to the next release milestone Jul 8, 2024
@kristinapathak kristinapathak deleted the builder-empty-gomod-field branch July 8, 2024 19:36
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.

[builder] more descriptive empty gomod error

3 participants