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

[featuregate] Deprecate NewFlag in favor of RegisterFlags #8727

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Oct 23, 2023

Description:

Deprecate featuregate.NewFlag in favor of Registry.RegisterFlags, which registers flags into a provided flagset.

The intent is to unblock future development of new features such as #7991 without incurring into deprecations or breaking changes to implement these.
This will allow us to do a 1.0 release.

Link to tracking Issue: Relates to #8220

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
otelcol/flags.go 100.00% <100.00%> (ø)
featuregate/flag.go 93.75% <75.00%> (-6.25%) ⬇️

... and 6 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@mx-psi mx-psi marked this pull request as ready for review October 23, 2023 10:59
@mx-psi mx-psi requested review from a team and Aneurysm9 October 23, 2023 10:59
@mx-psi mx-psi mentioned this pull request Oct 23, 2023
@mx-psi mx-psi requested a review from atoulme October 23, 2023 11:01
featuregate/flag.go Outdated Show resolved Hide resolved
@atoulme
Copy link
Contributor

atoulme commented Oct 26, 2023

I think I need to simplify things. I will drop this requirement to run this with a CLI flag since it's so impractical and work my way with a programmatic change so I can test strict mode in a test. We can stop this, we're trying to make it work but it's just impractical and I have exhausted enough good will with this attempt.

@atoulme atoulme closed this Oct 26, 2023
@bogdandrutu bogdandrutu reopened this Oct 26, 2023
@bogdandrutu
Copy link
Member

bogdandrutu commented Oct 26, 2023

@atoulme I understand your frustration, but please don't close others PRs without checking with them, especially when is not an obvious reason :)

@atoulme
Copy link
Contributor

atoulme commented Oct 26, 2023

@bogdandrutu Oh I meant to help close this because @mx-psi only opened it to make my own PR whole as we're going for 1.0. No harm done. Just trying to make it explicit to @mx-psi his help is deeply appreciated, but I need to relieve him from his duty of pushing this work in ahead of the 1.0 release of featuregate. I'll go close my original PR as well.

@bogdandrutu
Copy link
Member

@mx-psi @atoulme I still believe the current NewFlag is not very extensible, so having a way to extend that may help in the future, see my comments.

@atoulme
Copy link
Contributor

atoulme commented Oct 26, 2023

Understood! I'll leave it be then. But I can decorrelate the work on strict mode from flags, for now at least.

@mx-psi mx-psi changed the title [featuregate] Deprecate NewFlag in favor of NewFlags [featuregate] Deprecate NewFlag in favor of RegisterFlags Oct 27, 2023
featuregate/flag.go Outdated Show resolved Hide resolved
@mx-psi
Copy link
Member Author

mx-psi commented Nov 2, 2023

@atoulme @TylerHelmuth @djaglowski @MovieStoreGuy PR changed a bit after back and forth with Bogdan, mind taking another look?

@bogdandrutu bogdandrutu merged commit 1dd5ac3 into open-telemetry:main Nov 6, 2023
30 of 31 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 6, 2023
mx-psi added a commit that referenced this pull request Nov 15, 2023
**Description:** 

Removes deprecated function `featuregate.NewFlag`. 

This PR MUST NOT be merged before v0.89.0 is released.

**Link to tracking Issue:** Follow up to #8727
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.

6 participants