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

Migrate Sidekiq::Web to a pure Rack application #3069

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@badosu
Copy link
Contributor

commented Jul 26, 2016

Migrate Sidekiq::Web to a pure Rack application to avoid sinatra as
dependency. rack-protection is still needed.

The application is mounted on top of Rack::Builder, mantaining all of
the previous http interface.

Rack apps being used:

  • Rack::File to serve assets
  • Rack::Session::Cookie, the secret can be configured via
    Sidekiq::Web.session_secret
  • Rack::Protection, same as before when using sinatra
  • Sidekiq::WebApplication, described below.

Sidekiq::WebApplication is a very simple rack application composed of a
Sidekiq::WebRouter and a Sidekiq::WebAction dispatcher. This terminology
was adopted to be able to mantain Sidekiq::Web as a Rack app.

The Router is heavily inspired on Rack::Router[0](and in many parts
identical, I don't know if this can be an issue), however not being retrocompatible.

The Action is a wrapper to provide convenience, DRY code and maintain
the old interface.

I tried to mantain most of the old application structure so that
customizations and monkey-patches are easily adjustable or even
further work be done to enforce retrocompatibility.

Testing welcome!

0: https://github.com/pjb3/rack-router

@badosu badosu force-pushed the badosu:remove-sinatra branch from 489dab1 to 6a31d22 Jul 26, 2016

@badosu badosu closed this Jul 26, 2016

@badosu badosu reopened this Jul 26, 2016

Migrate Sidekiq::Web to a pure Rack application
Migrate Sidekiq::Web a pure Rack application to avoid sinatra as
dependency. rack-protection is still needed.

The application is mounted on top of Rack::Builder, mantaining all of
the previous http interface.

Rack apps being used:

- Rack::File to serve assets
- Rack::Session::Cookie, the secret can be configured via
  Sidekiq::Web.session_secret
- Rack::Protection, same as before when using sinatra
- Sidekiq::WebApplication, described below.

Sidekiq::WebApplication is a very simple rack application composed of a
Sidekiq::WebRouter and a Sidekiq::WebAction dispatcher. This terminology
was adopted to be able to mantain Sidekiq::Web as a Rack app.

The Router is heavily inspired on Rack::Router[0] (and in many parts
identical), however not being retrocompatible.

The Action is a wrapper to provide convenience, DRY code and maintain
the old interface.

I tried to mantain most of the old application structures so that
customizations and monkey-patches are easily adjustable or even
further work be done to enforce retrocompatibility.

Testing welcome!

0: https://github.com/pjb3/rack-router

@badosu badosu force-pushed the badosu:remove-sinatra branch from 6a31d22 to 9ea167d Jul 26, 2016

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

Holy cow, this is amazing.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

This is a huge change so a couple of notes:

  1. Thank you for this work - it's really awesome.
  2. This should probably go out as Sidekiq 5.0, which dovetails nicely with Rails 5.0.
  3. Have you tested any Web UI extensions, like sidekiq-failures, sidekiq-cron, sidetiq, etc? I'm ok with minimal breaking changes if necessary but we should make an effort not to break 3rd party gems.
  4. The major area where people tweak Sinatra is for the session store: #2910 (comment). Not sure what to do about that.

Looks like the test suite is passing so that's a huge step forward.

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2016

Hi Mike, thank you very much for the kind words.

    1. Where is this work located?
    1. No, not yet, I mostly ever used sidetiq so I'll have to take a look at this later.
    1. I had to tweak this as well in the past, sharing a session with a Roda application. I'll see where I can expose more options.

There are 3 important things to consider as well:

  • I noticed there are no tests with regard to pagination and I could not test this in the UI yet, so it is the most fragile area.
  • As you already pointed out, a lot of people customize their Sidekiq Web options so I'll pay special attention with these use cases
  • Some code is shared with rack-router and I am not sure how this licensing stuff works.

That said: the ERB templates should be cached and the configuration flexibility is still lacking. I just pulled this in an all-nighter so I'll take a look at these refinements later on.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

Sidekiq 5.0 does not exist yet. This branch + the rails5 branch can be its start.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

pagination testing can be as simple as 100.times {|idx| HardWorker.perform_async(idx) } then navigate to the default queue.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

rack-router is MIT licensed. If a non-trivial amount of rack-router code is included, we need to add a Credits.txt with its MIT license.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

Sinatra's hooks for registering extensions are used by Sidekiq Pro, Sidekiq Enterprise and a lot of 3rd party extensions. We'll need to add support for them. I can help but that appears to be the biggest blocker right now.

/Users/mike/src/pro/lib/sidekiq/pro/web.rb:119:in <top (required)>': undefined methodregister' for Sidekiq::Web:Class (NoMethodError)

::Sidekiq::Web.register Sidekiq::Pro::Web
@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

I've merged these changes into the badosu-remove-sinatra branch.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

@badosu If you want committer access so you can polish these changes more, just let me know.

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2016

@mperham Could you give me that access? I'll continue on those points tonight.

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2016

@mperham I've made some improvements and the sidetiq page is already accessible. Some points though:

  • It is not needed anymore to include explicitly home_path on redirect calls, as seen here
  • I had to reinclude code to set local variables inside partials, however It is not compatible with ruby 2.0.0 and I can't see a way to do it without using eval.
  • params includes only the querystring parameters accessible via String keys instead of symbols
  • route_params includes those that come from the route, accessible via symbols

We will have to decide what to deprecate and what not, fortunately it looks pretty straightforward to upgrade old extensions to the new model.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

Could you open up a new PR for the branch in this repo so we can see commits and discuss?

@mperham mperham closed this Jul 27, 2016

@badosu badosu deleted the badosu:remove-sinatra branch Jul 27, 2016

@natebird

This comment has been minimized.

Copy link

commented on 9ea167d Jul 28, 2016

Wow. I expected much more changes than this for a port to Rack. This is very nice. It fits right in with Sidekiq's reduce dependencies motto.

end

def initialize
secret = Web.session_secret

This comment has been minimized.

Copy link
@mperham

mperham Aug 18, 2016

Owner

This requires the user to set the session_secret manually to interoperate with an existing session store. Could we pull it out of the Rails app to make Sidekiq work without any user intervention, e.g.

def retrieve_secret
  sec = nil
  sec ||= Web.session_secret
  sec ||= Rails.application.session_secret if defined?(::Rails) # not sure of the syntax in Rails 3.2, 4.x, 5.x...
  sec ||= SecureRandom.hex(64)
  sec
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.