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

Implements namespaces #268

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Conversation

zedtux
Copy link
Contributor

@zedtux zedtux commented Jan 17, 2020

Namespacing

This gem can isolate jobs within namespaces.

Default namespace

When not giving a namespace, the default one will be used.

In the case you'd like to change this value, create a new initializer like so:

config/initializers/sidekiq-cron.rb:

Sidekiq::Cron.configure do |config|
  config.default_namespace = 'statics'
end

Usage

When creating a new job, you can optionaly give a namespace attribute, and then you can pass it too in the find or destroy methods.

Sidekiq::Cron::Job.create(
  name: 'Hard worker - every 5min',
  namespace: 'Foo',
  cron: '*/5 * * * *',
  class: 'HardWorker'
)
# INFO: Cron Jobs - add job with name Hard worker - every 5min in the namespace Foo

# Without specifing the namespace, Sidekiq::Cron use the `default` one, therefore `count` return 0.
Sidekiq::Cron::Job.count
#=> 0

# Searching in the job's namespace returns 1.
Sidekiq::Cron::Job.count 'Foo'
#=> 1

# Same applies to `all`. Without a namespace, no jobs found.
Sidekiq::Cron::Job.all

# But giving the job's namespace returns it.
Sidekiq::Cron::Job.all 'Foo'
#=> [#<Sidekiq::Cron::Job:0x00007f7848a326a0 ... @name="Hard worker - every 5min", @namespace="Foo", @cron="*/5 * * * *", @klass="HardWorker", @status="enabled" ... >]

# If you'd like to get all the jobs across all the namespaces then pass an asterisk:
Sidekiq::Cron::Job.all '*'
#=> [#<Sidekiq::Cron::Job ...>]

job = Sidekiq::Cron::Job.find('Hard worker - every 5min', 'Foo').first
job.destroy
# INFO: Cron Jobs - deleted job with name Hard worker - every 5min from namespace Foo
#=> true

Sidekiq web update

A new bar has been added showing the namespaces, allowing to switch namespaces:

Screenshot 2020-01-21 at 09 19 04

Notes

I haven't updated the slim files as slim has been removed 7 years ago from sidekiq, but if you really want it, I can update them.

@coveralls
Copy link

coveralls commented Jan 17, 2020

Coverage Status

Coverage increased (+0.8%) to 92.275% when pulling 4ce54b3 on Pharmony:features/namespaces into 7ca90f9 on ondrejbartas:master.

@zedtux zedtux force-pushed the features/namespaces branch 3 times, most recently from aca9e32 to ad65232 Compare February 26, 2020 13:16
@ondrejbartas
Copy link
Collaborator

@zedtux Very nice code. I like your approach. Can you rebase your code to master and change slim templates? I will like to release it with new version

@zedtux
Copy link
Contributor Author

zedtux commented Apr 3, 2020

Thank you @ondrejbartas. Sure I'll do it.

For your information, this code is running in our prod since weeks.

@zedtux
Copy link
Contributor Author

zedtux commented Apr 4, 2020

@ondrejbartas it's done !

@zedtux
Copy link
Contributor Author

zedtux commented Apr 4, 2020

Don't know what is happening ... Github seem to have missed the Travis CI callback to set the build as passed 🤷‍♂.

@zedtux
Copy link
Contributor Author

zedtux commented Apr 9, 2020

@ondrejbartas can you please have a look?

@zedtux
Copy link
Contributor Author

zedtux commented Apr 16, 2020

@ondrejbartas please?

@zedtux
Copy link
Contributor Author

zedtux commented Nov 23, 2021

@ondrejbartas is there any chance this get merged one day or should I wast my time in building a new gem?

@honzasterba
Copy link
Collaborator

@zedtux please update the PR to latest master, would love to merge this

@zedtux zedtux force-pushed the features/namespaces branch 3 times, most recently from 182da92 to 8aa0a91 Compare April 7, 2022 07:24
@honzasterba honzasterba self-requested a review April 7, 2022 07:43
@zedtux zedtux force-pushed the features/namespaces branch 9 times, most recently from f2bc3df to ed6bd2a Compare April 8, 2022 07:59
@zedtux
Copy link
Contributor Author

zedtux commented Apr 8, 2022

@honzasterba all conflicts fixed, can you please merge it now ?

Copy link
Collaborator

@honzasterba honzasterba left a comment

Choose a reason for hiding this comment

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

missin unit tests

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/sidekiq/cron.rb Outdated Show resolved Hide resolved
lib/sidekiq/cron/job.rb Show resolved Hide resolved
@zedtux zedtux force-pushed the features/namespaces branch 2 times, most recently from cd6cffe to 53e453d Compare April 26, 2022 12:45
@zedtux
Copy link
Contributor Author

zedtux commented Jul 17, 2023

Use a more official image (Alpine Ruby?)

I'm not sure what you mean here, ruby:2.7 refers to the officiel Ruby docker image registry : https://hub.docker.com/_/ruby. The Alpine version is a smaller version as a label from the same repository.

I could update it, if you really have an interest for it, but in my honest opinion, as this is only a development image, it doesn't matter much.
Image size is more about production environment in order to speed-up the deployment.

remove MAINTAINER Joao Serra joaopfserra@gmail.com (he is not maintaining anything in this organization)

Sure, it has to be changed to something else. Like you ? Or you don't want to specify a MAINTAINER?

Move Docker related files to a folder (docker/*) to avoid polluting the root workspace

I'm used to keep them at the root so that the commands are easier but I can understand your point of view since you're not a docker user.

Document how to use it (probably under this section https://github.com/sidekiq-cron/sidekiq-cron#contributing)

I will do it with pleasure.

Otherwise, about this PR, can you do the code review and merge or is it still @honzasterba who's doing it?

@markets
Copy link
Member

markets commented Jul 17, 2023

ah ok! understood! let me clarify some points about Docker:

  • Agree to use official Ruby image, I was confused by the MAINTAINER tag probably and my little knowledge about Docker. Maybe we can use a newer version btw, like 3.2?
  • If the MAINTAINER tag is not mandatory, I'd just ✂️
  • If having those files them under docker/* is not a problem or a bad practice in the ecosystem, I personally value having a clean root folder

About the code changes, since this patch is quite big, I'd like more eyes on it. Let me ping some people involved in this project for the latest years/releases. @honzasterba @ondrejbartas @jmettraux @serprex @engwan @petergoldstein @ybiquitous could you please take a look to this or test it locally when you have a chance 🙏🏼? Thank you in advance!

And thanks again to @zedtux for you amazing work 👏🏼 on this feature and your patience.

@zedtux
Copy link
Contributor Author

zedtux commented Jul 17, 2023

Okay, so let's remove the MAINTAINER instruction, upgrade to ruby 3.2 and move files in a docker/ folder.

@zedtux
Copy link
Contributor Author

zedtux commented Jul 31, 2023

Could anyone do the code review please, I really would love to see this merged! 😍

lib/sidekiq/cron/job.rb Outdated Show resolved Hide resolved
lib/sidekiq/cron/job.rb Outdated Show resolved Hide resolved
lib/sidekiq/cron/job.rb Outdated Show resolved Hide resolved
@zedtux
Copy link
Contributor Author

zedtux commented Sep 6, 2023

Okay, so let's remove the MAINTAINER instruction, upgrade to ruby 3.2 and move files in a docker/ folder.

This has been done too!

@zedtux
Copy link
Contributor Author

zedtux commented Sep 6, 2023

I don't understand why build matrix is red now, it doesn't look related to my changes... anyone has an idea please?

@serprex
Copy link
Contributor

serprex commented Sep 6, 2023

Regression caused by recent change to minitest removing some deprecated constant

freerange/mocha#614

Fixed by #420

@serprex
Copy link
Contributor

serprex commented Sep 7, 2023

@zedtux CI fixed, try updating PR with latest

@zedtux
Copy link
Contributor Author

zedtux commented Sep 8, 2023

Thanks @serprex, the build is now green. 👍

@zedtux
Copy link
Contributor Author

zedtux commented Sep 8, 2023

@honzasterba @markets the PR is now ready, any other review or are we good to go?

@markets
Copy link
Member

markets commented Sep 8, 2023

Seems fine to me @zedtux 👍🏼 I'll try to merge it next week and it will be released as v2.0 soon. Sorry I'm on parental leave and my free time is limited until mid october.

Could you please add some docs about how to use docker support?

@zedtux
Copy link
Contributor Author

zedtux commented Nov 10, 2023

Hello there,

Any news please ?

@markets
Copy link
Member

markets commented Nov 13, 2023

Hi @zedtux, sorry the past 2 weeks we have been working in an relevant fix (more info #428) introduced some months ago.

I think we are now ready to merge this one, targeting a future v2 release (should be the next one and I'd like to launch this as v2-beta).

Let me ask a couple of details 🙏🏼:

@zedtux zedtux force-pushed the features/namespaces branch 2 times, most recently from 43f9a32 to 0e5a0f1 Compare December 18, 2023 13:35
@zedtux
Copy link
Contributor Author

zedtux commented Dec 18, 2023

Let me ask a couple of details 🙏🏼:

Hey @markets,

  1. Git rebase done
  2. Docker doc written
  3. Screenshot updated

All should be alright now. Let's merge it!

@markets
Copy link
Member

markets commented Dec 21, 2023

Thanks again @zedtux 👍

I'd like to merge it during the christmas holidays and probably cut a release as v2.0.0-beta.

@zedtux
Copy link
Contributor Author

zedtux commented Jan 2, 2024

@markets happy new year, let's get this PR merged! 🥳

@markets markets merged commit 160d34d into sidekiq-cron:master Jan 2, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants