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

Merge Bundle and ConfiguredBundle #2512

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@alex-shpak
Copy link
Contributor

alex-shpak commented Sep 30, 2018

Problem:

Right now there are two types of bundles Bundle and ConfiguredBundle and it looks like former was added to maintain backward compatability.
As of approaching 2.0.0 release I would like to propose to merge them and keep only ConfiguredBundle

There are few related discussions: #1701 (comment) and #1360

Note that change is breaking to all existing bundles

Solution:

In short terms:

  1. Bundle interface is gone
  2. ConfiguredBundle interface is renamed to Bundle
  3. Related code adjusted
Result:

There will be only one bundle to extend from which will make bundles simpler,
This will also fix bundle run ordering.

TODO:
  • Update documentation
@jplock

This comment has been minimized.

Copy link
Member

jplock commented Oct 1, 2018

@dropwizard/committers do we want to do this? I maintain some bundles that don't require any configuration where I'm using Bundle. I think this would cause a lot of churn for people waiting for their bundles to be updated.

@evnm

This comment has been minimized.

Copy link
Member

evnm commented Oct 1, 2018

Sounds like a job for an @Deprecated annotation.

@jplock

This comment has been minimized.

Copy link
Member

jplock commented Oct 1, 2018

@evnm @alex-shpak I'd be in favor of releasing 1.3.7 with Bundle deprecated and then remove it in 2.0.0, but I don't think we should remove in 2.0.0 without deprecating first. Or we deprecate in 2.0.0 and remove it in 2.1.0 or something.

@evnm

This comment has been minimized.

Copy link
Member

evnm commented Oct 1, 2018

Removing it would warrant a major version bump, so it seems to me like the right thing to do is your first option, @jplock—deprecate in 1.3.7 and remove in 2.0.0.

@alex-shpak

This comment has been minimized.

Copy link
Contributor

alex-shpak commented Oct 2, 2018

Also we can start with way proposed in #1360 and mark Bundle as @Deprecated and then remove it later.

joschi added a commit that referenced this pull request Oct 3, 2018

Deprecate Bundle in favor of ConfiguredBundle<T>
Instead of supporting two types of bundles, `Bundle` and `ConfiguredBundle<T>`, this change deprecates `Bundle` in favor of `ConfiguredBundle<T>` in a backward-compatible way.

Closes #1360
Closes #2512
@joschi

This comment has been minimized.

Copy link
Member

joschi commented Oct 3, 2018

I've given the thing a try in #2516 with a backward-compatible change.

@jplock jplock closed this in #2516 Oct 3, 2018

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