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.

Sign up for GitHub

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

Jump to bottom

Support AJ 7.1 perform_all_later api #110

Merged
merged 4 commits into from
Jan 19, 2024
Merged

Support AJ 7.1 perform_all_later api #110

merged 4 commits into from
Jan 19, 2024
+213 −39

Conversation

kakubin
Copy link
Contributor

@kakubin kakubin commented Jan 17, 2024 •
edited

@kakubin kakubin marked this pull request as ready for review January 17, 2024 15:28
@kakubin kakubin force-pushed the support_rails_7_1_perform_all_later_api branch from 0824f68 to f7af45c Compare January 17, 2024 15:38
@kakubin kakubin force-pushed the support_rails_7_1_perform_all_later_api branch from f7af45c to 067f6a3 Compare January 17, 2024 15:41
mullermp
mullermp reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Thanks for opening a pull request. This is a great first start!

test/active_job/queue_adapters/amazon_sqs_adapter_test.rb
@@ -188,6 +188,30 @@ module QueueAdapters
end.to raise_error ArgumentError
end
end

if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

One option we can consider is major version bumping aws-sdk-rails to hard depend on Rails 7.1, and dropping old dependencies. I would not be against this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has no effect on the old rails version, so it could be tagged as minor change.
but if there is no other opportunity like this PR(supports Rails 7.1 feature), I think it would be a good idea to cut off old version's support and major bumping with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll talk with the team and get back to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a minor version release is fine for this. Checking the dependencies, they're already loose.

Gemfile Outdated Show resolved Hide resolved
@kakubin kakubin force-pushed the support_rails_7_1_perform_all_later_api branch from 067f6a3 to 483d01c Compare January 17, 2024 23:15
@kakubin kakubin force-pushed the support_rails_7_1_perform_all_later_api branch 2 times, most recently from 641f944 to ad7856a Compare January 17, 2024 23:36
@kakubin
feature: add api for queuing multiple jobs
1941d8d 
this supports Rails 7.1 feature `ActiveJob::Base.perform_all_later`
@kakubin kakubin force-pushed the support_rails_7_1_perform_all_later_api branch from ad7856a to 1941d8d Compare January 18, 2024 00:30
@kakubin kakubin requested a review from mullermp January 18, 2024 00:30
@kakubin kakubin changed the title Support rails 7 1 perform all later api Support AJ 7.1 perform all later api Jan 18, 2024
@mullermp
Copy link
Contributor

mullermp commented Jan 18, 2024 •
edited

Would you be interested in adding a real "integration test" for this feature? This repo should have a sample rails app that minimally sets up each basic feature for testing. It would be nice to have a basic working example and document it too.

mullermp
mullermp approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

test/active_job/queue_adapters/amazon_sqs_adapter_test.rb
@@ -188,6 +188,30 @@ module QueueAdapters
end.to raise_error ArgumentError
end
end

if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a minor version release is fine for this. Checking the dependencies, they're already loose.

alextwoods
alextwoods approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@alextwoods alextwoods 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 for the contribution!

@kakubin
Copy link
Contributor Author

kakubin commented Jan 19, 2024

@mullermp

Would you be interested in adding a real "integration test" for this feature? This repo should have a sample rails app that minimally sets up each basic feature for testing. It would be nice to have a basic working example and document it too.

I'll do 👍
Are you saying adding specs after this PR merged?
Also, if I add spec for this feature, we need to upgrade Rails version on sample_app/.

@kakubin kakubin changed the title Support AJ 7.1 perform all later api Support AJ 7.1 perform_all_later api Jan 19, 2024
@mullermp mullermp merged commit a933b7a into aws:main Jan 19, 2024
39 checks passed
@mullermp
Copy link
Contributor

Yes - if you'd like to help upgrade the sample app and add a small job that demonstrates the feature, that would be very helpful. The sample app is very minimal so I don't see issue with upgrading it.

@kakubin kakubin deleted the support_rails_7_1_perform_all_later_api branch January 20, 2024 01:00
@kakubin kakubin mentioned this pull request Apr 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mullermp mullermp mullermp approved these changes

@alextwoods alextwoods alextwoods approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for ActiveJob.perform_all_later (Rails 7.1)
3 participants
@kakubin @mullermp @alextwoods