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

Switch to minitest and convert the server to a Rails engine to match new plugins structure for Resque-Web #464

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

mattgibson
Copy link

I've so far only done the bare minimum here, so there is more to be done to tidy things up.

So far, I have added a Rails engine skeleton (including the dummy app for tests), then found out that the dummy app includes Minitest for Rails 4, which is incompatible with Test::Unit. This necessitated refactoring all of the test suite to use Minitest, then I moved all of the existing code from the server into the engine and made sure it worked.

It all seems to run smoothly, but I'm still testing some parts that do not appear to have had full test coverage before, so I've made this PR to solicit input.

Questions:

  1. Does this look OK so far (I am not very familiar with Test::Unit, or Minitest, or resque-scheduler's code base)
  2. Anything obvious that needs altering/improving?
  3. What more would be needed for this PR to be merged?

I'm planning on adding a few integration tests to make sure all the links etc work when clicked, as well as verifying stuff that should be visible or not on screen.

@rpechayr
Copy link

👍

@rpechayr
Copy link

This is really great but if I understand correctly, your PR completely removes support for the old sinatra based of resque. The only reason why I still care about this sinatra app is because Resque Scheduler does not support the new one. However I think there should be a way to support both until Resque completely removes the sinatra app in resque 2.0.

I think an efficient way of making your work usable right now would be to make a gem on top of resque-scheduler and resque-web. I think it could ultimately be available on github at resque/resque-scheduler_web

What do you think ?
cc @meatballhat

@Ahrry
Copy link

Ahrry commented Feb 23, 2015

@rpechayr IMO this is definitely the way to go !

@mattgibson
Copy link
Author

@rpechayr Thanks. I removed the Sinatra one as it seemed unnecessary alongside the new implementation and adding it back would mean rewriting all the tests for it in Minitest. This seemed like a lot of work when it's on the way to deprecation.

You say "Resque Scheduler does not support the new one", but that's what this PR achieves, so I'm not quite sure what you mean there. Are there cases where the old Sinatra app will still need to be used even if this PR is merged? If so, is it an option to bump Resque Scheduler by a major version to signal a breaking change?

Happy to make a new gem if that's the easiest way to go.

@rpechayr
Copy link

@mattgibson sorry if I was not clear before. My only problem with resque/resque-web is that resque-scheduler is not supporting it. Otherwise I would already be using resque/resque-web in all my projects.
Your PR does what I want. However, Resque 2.0 has not been released yet so the sinatra app Resque provides is the default resque web UI. When Resque 2.0 is released they will drop the sinatra plugin and use resque/resque-web only. At this point, resque-scheduler will have to support resque/resque-web in itself to be compatible with Resque (which is what your PR does).

What I was suggesting is that your code seems already self-sufficient to create a separate gem. Imagine you release this gem tomorrow (with Resque , Resque Scheduler, and resque-web as dependencies), people like me using resque scheduler will use it the day after tomorrow with the resque-web gem. Otherwise if it is part of Resque Scheduler, no version of resque scheduler can be released with your contribution until Resque 2.0 is released. I have no idea when Resque 2.0 will be released though ...

@mattgibson
Copy link
Author

@rpechayr Ah, I see what you mean. Yes, that does make sense. Resque 2.0 seems to be in limbo from my reading of resque/resque#1170 so no telling when it will arrive.

I'll make a start on a separate gem.

@rpechayr
Copy link

@mattgibson sounds good. Let me know of how it goes. Sorry I can't help you for the moment. Seems like most of the work is already done ;)

'engine/app/controllers'
end

Engine.routes do

Choose a reason for hiding this comment

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

How different is this from describing routes in a dedicated config/routes.rb file ?

@MrHubble
Copy link

For someone just starting with resque, resque-scheduler and resque-web, what versions should I be using to get the "Schedule" tab on resque-web?

From my understanding the current version of resque to use in production is 1.25.2. In the discussion above @rpechayr states:

Resque 2.0 has not been released yet so the sinatra app Resque provides is the default resque web UI.

so I feel I should be using the default sinatra resque-web and not using the rails version of resque/resque-web, and version 4.0.0 of resque-scheduler should work correctly at showing the tab within resque-web. Is this correct?

@rpechayr
Copy link

Yes this Is correct.

Sent from my iPhone

On 27 Feb 2015, at 06:00, MrHubble [email protected] wrote:

For someone just starting with resque, resque-scheduler and resque-web, what versions should I be using to get the "Schedule" tab on resque-web?

From my understanding the current version of resque to use in production is 1.25.2. In the discussion above @rpechayr states:

Resque 2.0 has not been released yet so the sinatra app Resque provides is the default resque web UI.

so I feel I should be using the default sinatra resque-web and not using the rails version of resque/resque-web, and version 4.0.0 of resque-scheduler should work correctly at showing the tab within resque-web. Is this correct?


Reply to this email directly or view it on GitHub.

@mattgibson
Copy link
Author

@rpechayr Here's a first go at a gem: https://github.com/mattgibson/resque-scheduler-web

There's a bit of test coverage missing, but all the old tests are there, and ported over to Rspec. I'm going to add more tests to cover 100% of the functionality and then look at refactoring.

@rpechayr
Copy link

Great ! I'll definitely have a look !

Sent from my iPhone

On 11 Apr 2015, at 18:22, Matt Gibson [email protected] wrote:

@rpechayr Here's a first go at a gem: https://github.com/mattgibson/resque-scheduler-web

There's a bit of test coverage missing, but all the old tests are there, and ported over to Rspec. I'm going to add more tests to cover 100% of the functionality and then look at refactoring.


Reply to this email directly or view it on GitHub.

@markets
Copy link

markets commented May 19, 2015

Very nice job @mattgibson 👍, I just was starting to write a patch to add an endpoint to remove a delayed job and I found this PR and your gem, which includes that functionality https://github.com/mattgibson/resque-scheduler-web/blob/master/app/controllers/resque_web/plugins/resque_scheduler/delayed_controller.rb#L41-L49.

That is really a useful feature!

If I understand correctly, we should wait Resque 2 release to move forward regarding this part, no? Thanks!

@mattgibson
Copy link
Author

@markets Thanks. Glad it's useful :)

Yes, the consensus seems to be that this should not be in Resque Scheduler until Resque 2, but in the meantime, I have added a number of improvements to the code in the gem that are not in this PR. One big one being porting the test suite to Rspec, then getting it up to around 97% coverage with integration tests, but also improvements to the UI and cleaning up some internal code.

I'm not sure what the best strategy is for moving those changes back into Resque Scheduler in future, or whether keeping them as a separate gem makes more sense.

@mattgibson
Copy link
Author

@markets If you meant "does this work with Resque 1.x right now", then yes, it does - just use the resque-web gem and my gem together instead of the Sinatra based server and it works fine.

@markets
Copy link

markets commented May 19, 2015

Thanks for clarifying @mattgibson, good job with that lib: better test suite and coverage, code clean up, new features... 🔝 IMHO would be more natural to move back those changes into this repo (or at least, host it under Resque Organization, kind of "official" stuff), meanwhile I'm going to install your gem 😃 Thank you guys!

Probably a different repo for UI make sense, similar to resque-web:

resque/resque (backend) => resque/resque-web (frontend)
resque/resque-scheduler (backend) => resque/resque-scheduler-web (frontend)

@rpechayr
Copy link

@mattgibson @markets In the future Resque web will be a separate gem living outside of resque. I would then think that keeping @mattgibson gem separetely make more sense.

@rpechayr
Copy link

I also totally agree with @markets that this gem should be in the resque organization.

@mattgibson
Copy link
Author

@rpechayr @markets I agree it would make sense to move it to the Resque organisation. Not sure what the normal procedure for getting that to happen is, though. I'm happy to keep maintaining it once it's there.

@meatballhat
Copy link
Member

Sorry for the long delay! I'm working my way through PRs right now. Any additional thoughts on this before I attempt to get it merged, which will likely result in a bump to v5.0.0 and pegging v4.x as the final Resque 1.x (+Sinatra) major version?

@meatballhat
Copy link
Member

@mattgibson bump 👋 😸

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

Successfully merging this pull request may close these issues.

6 participants