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

32669 sumconnector config #33994

Merged

Conversation

greatestusername
Copy link
Contributor

@greatestusername greatestusername commented Jul 9, 2024

Description: Fleshes out the config more fully and provides tests for the config and possible combinations of settings

Link to tracking Issue: #32669

Testing: Added test cases and testing for config

Documentation: No new documentation yet.

@greatestusername
Copy link
Contributor Author

greatestusername commented Jul 9, 2024

Regarding tests and the size of PRs:

Is it cool to do tests in a separate PR from functionality?
Realizing that the tests for this small bit were like 500 lines and I'm sure you don't want to look at things much larger than that in an entire PR if possible 😛

Edit to add: Is changelog required here if the component isn't live or wired in yet?

@greatestusername greatestusername marked this pull request as ready for review July 9, 2024 19:02
@greatestusername greatestusername requested a review from a team July 9, 2024 19:02
@crobert-1
Copy link
Member

crobert-1 commented Jul 9, 2024

Regarding tests and the size of PRs:

This is fine for me since it's mostly test code. My preference is to keep functionality changes and the relevant tests in the same PR as much as possible. I'll review.

As far as changelog, it's really just what's relevant to users. Announcing that a new component is being adding makes sense (the last PR), adding config validation not as much. I don't think a changelog is required here, but you're welcome to add one if you'd like.

@crobert-1 crobert-1 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 9, 2024
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Show resolved Hide resolved
@crobert-1
Copy link
Member

From failing CI actions:

Generated code is out of date, please run "make generate" and commit the changes in this PR.
sumconnector/config_test.go:510: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
			expect: `spans: metric name missing`+"\n"+`spans: metric source_attribute missing`+"\n"+`spans condition: metric "": unable to parse OTTL condition "invalid condition": condition has invalid syntax: 1:9: unexpected token "condition" (expected <opcomparison> Value)`+"\n"+`spans attributes: metric "": attribute key missing`,
sumconnector/config_test.go:525: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
			expect: `spanevents: metric name missing`+"\n"+`spanevents: metric source_attribute missing`+"\n"+`spanevents condition: metric "": unable to parse OTTL condition "invalid condition": condition has invalid syntax: 1:9: unexpected token "condition" (expected <opcomparison> Value)`+"\n"+`spanevents attributes: metric "": attribute key missing`,
sumconnector/config_test.go:540: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
			expect: `metrics: metric name missing`+"\n"+`metrics: metric source_attribute missing`+"\n"+`metrics condition: metric "": unable to parse OTTL condition "invalid condition": condition has invalid syntax: 1:9: unexpected token "condition" (expected <opcomparison> Value)`+"\n"+`metrics attributes not supported: metric ""`,

@crobert-1
Copy link
Member

Test failures are frequencies of #33998, added reference there.

@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Jul 16, 2024
@codeboten codeboten merged commit 358ab68 into open-telemetry:main Jul 25, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector/sum ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants