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 update main image background job to sidekiq #5345

Conversation

codekatas-magmalabs
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Feature
  • Optimization

Description

Related Tickets & Documents

#5305

Added to documentation?

  • [x ] no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

flanders

This is a port of app/jobs/update_main_image_background_job.rb
that works with Sidekiq instead of DelayedJob.
This updates all references to UpdateMainImageBackgroundHexJob
so they now call UpdateMainImageBackgroundHexWorker, which works
with Sidekiq instead of DelayedJob.
@codekatas-magmalabs codekatas-magmalabs requested a review from a team January 3, 2020 03:51
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 3, 2020
@claassistantio
Copy link

claassistantio commented Jan 3, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Hi @codekatas-magmalabs, thanks a lot for your PR!

I think the code is fine, maybe we'll have to reconsider the priority but overall it's a solid PR.

There's one failing test though, see https://travis-ci.com/thepracticaldev/dev.to/builds/142954923, the reason why it's failing it's because the test is using assert_enqueued_with which is based on ActiveJob, but we're not using that anymore. You can replace it with sidekiq_assert_enqueued_with, see my comment here: #5305 (comment)

Thank you!

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 3, 2020
@rhymes
Copy link
Contributor

rhymes commented Jan 3, 2020

Also, don't forget to sign the CLA up here

@atsmith813
Copy link
Contributor

Hey @codekatas-magmalabs, thanks for the contribution! Would you like to update the specs as Rhymes mentioned? I know life has funny ways of getting busy. I'm happy to update it or assist as well if that's easier, just let me know. Thanks!

@@ -429,7 +429,7 @@ def evaluate_markdown
def update_main_image_background_hex
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also being triggered in an after_save callback which means the record will not be in the database yet if it is new. Can you change this so it's fired in an after_commit callback? THANK YOU!

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Feb 4, 2020
@atsmith813
Copy link
Contributor

@codekatas-magmalabs I went ahead and pushed up the requested changes. We're in the home stretch of finishing off this epic (#5305). Thank you so much for getting this going and please let me know if you have any questions!

@mstruve @rhymes I've made the requested changed!

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @codekatas-magmalabs and @atsmith813!

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 4, 2020
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@atsmith813 atsmith813 merged commit bae7ee8 into forem:master Feb 5, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants