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

feat(delayed job): allow to select the modules as part of the jobs name #314

Merged

Conversation

martinramirez7
Copy link
Contributor

This PR allows to use modules as part of jobs names on delayed jobs metrics.

This is useful in apps with several namespaces where each defines the same set of jobs, all running in the same queue, which is our situation.

It ensures backwards compatibility.

@martinramirez7
Copy link
Contributor Author

Hey @SamSaffron, can you take a quick look of this please? It seems linter is failing on unchanged files

@SamSaffron
Copy link
Member

any chance you can add a test to properly clarify new behavior ?

@martinramirez7 martinramirez7 force-pushed the add-module-name-for-delayed-jobs-names branch from ea834d4 to 0e59eb4 Compare April 9, 2024 16:22
@martinramirez7
Copy link
Contributor Author

Hey Sam, just added the new param to some tests. I don't really understand why the CI's tests are failing, any idea? locally works fine. I believe it has nothing to do with my changes. let me know if I'm wrong

@blackjid
Copy link
Contributor

blackjid commented Apr 9, 2024

@martinramirez7 the specs are failing in master also, something changed. But is should be fixed if you add require 'ostruct' to the /test/server/runner_test.rb file.

@martinramirez7
Copy link
Contributor Author

@SamSaffron I added a commit that fixes the CI problem with @blackjid suggestion. The problem is due json 2.7.2 doesn't load open struct library by default as it was on 2.7.1. you can see this PR for mor info

@martinramirez7
Copy link
Contributor Author

hey @SamSaffron friendly reminder of this PR

@martinramirez7 martinramirez7 force-pushed the add-module-name-for-delayed-jobs-names branch from 9381307 to 0e59eb4 Compare May 30, 2024 20:53
@blackjid
Copy link
Contributor

blackjid commented Jun 6, 2024

@SamSaffron any chance to merge and release this? That would be awesome

@SamSaffron
Copy link
Member

thanks ! merged.

@SamSaffron SamSaffron merged commit 1d8e6b1 into discourse:main Jun 19, 2024
13 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants