Skip to content

subscriber: make reload an optional feature #1286

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.

Sign up for GitHub

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

Jump to bottom
Open
Folyd wants to merge 2 commits into
base: master
Choose a base branch
from
Open

subscriber: make reload an optional feature #1286

Folyd wants to merge 2 commits into from

Conversation

@Folyd
Copy link
Contributor

@Folyd Folyd commented Mar 9, 2021

Motivation

reload is a useful feature for some applications such as web-app
but is unnecessary for the others such as CLI-app. I think this feature
should be optional.

Solution

Just add a new feature flag called reload and make it optional.

@Folyd Folyd requested review from a team, davidbarsky and hawkw as code owners March 9, 2021 12:01
@Folyd Folyd force-pushed the reload-feature branch 2 times, most recently from 2d745d8 to 6c6eecb Compare March 9, 2021 14:53
davidbarsky
davidbarsky requested changes Mar 9, 2021
View reviewed changes
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

Thanks for opening this! Had a few nits.

@Folyd
Copy link
Contributor Author

Folyd commented Oct 25, 2021

Orz, this PR didn't catch the v0.3 release. 😅 Can we merge this?

@davidbarsky
Copy link
Member

davidbarsky commented Nov 24, 2021

Sorry! We missed this change for 0.3. The chrono vulnerability was somewhat urgent and we missed a few breaking changes that we wanted to get it. Somewhat naively, I worry that maintaining this on master while not backporting this to v0.1.x will be somewhat annoying. If it's okay with you, I'd like to keep this PR open, but mark it with a tracing-subscriber-0.4 tag so that we don't forget to merge it ahead of that release.

@Folyd
Copy link
Contributor Author

Folyd commented Nov 25, 2021

Thanks, @davidbarsky. 😸

@bryangarza bryangarza added this to the tracing-subscriber 0.4 milestone May 9, 2022
@bryangarza
Copy link
Member

bryangarza commented May 9, 2022

Added the tracing-subscriber 0.4 milestone to this issue, I think this is still a valid candidate for getting merged. Let's keep it open for now.

cc @davidbarsky

@hawkw
Copy link
Member

hawkw commented May 9, 2022

Yeah, I'd like to consider a breaking release for a couple other changes, so when we do one, we should make sure this actually gets in too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidbarsky davidbarsky davidbarsky requested changes

@hawkw hawkw Awaiting requested review from hawkw hawkw is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

tracing-subscriber 0.4

Development

Successfully merging this pull request may close these issues.

4 participants

@Folyd @davidbarsky @bryangarza @hawkw