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

failed on unregistered featuregate #5660

Merged

Conversation

fatsheep9146
Copy link
Contributor

@fatsheep9146 fatsheep9146 commented Jul 9, 2022

Signed-off-by: Ziqi Zhao [email protected]

Description: <Describe what has changed.
Enhance registry to support failing on unregistered featuregate

fix #5656

@fatsheep9146 fatsheep9146 requested review from a team and codeboten July 9, 2022 15:54
@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #5660 (cc741d3) into main (58e66af) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5660   +/-   ##
=======================================
  Coverage   91.41%   91.42%           
=======================================
  Files         191      191           
  Lines       11386    11398   +12     
=======================================
+ Hits        10409    10421   +12     
  Misses        778      778           
  Partials      199      199           
Impacted Files Coverage Δ
service/featuregate/gates.go 100.00% <100.00%> (ø)
service/telemetry.go 89.20% <0.00%> (ø)
service/featuregate/flags.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58e66af...cc741d3. Read the comment docs.

Signed-off-by: Ziqi Zhao <[email protected]>
CHANGELOG.md Outdated
@@ -5,6 +5,7 @@
### 💡 Enhancements 💡

- Add `linux-ppc64le` architecture to cross build tests in CI
- Collector failed when passed an unregistered feature gate (#5660)
Copy link
Member

Choose a reason for hiding this comment

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

Please also add an entry in the Deprecated section for the deprecated Apply function

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 entry is supplied.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

One question, please add a unit test as well to ensure the functionality works as expected

service/featuregate/gates.go Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

@fatsheep9146 can you also add a new test(s) that verify the collector fails to start on an unknown feature flag

@fatsheep9146
Copy link
Contributor Author

@TylerHelmuth @codeboten
I've already added a unit test for registered and unregistered gate case.

service/command.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@codeboten codeboten merged commit 1b32ae0 into open-telemetry:main Jul 12, 2022
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.

Collector should fail when passed an unregistered feature gate
4 participants