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

[5.x] Add ability to specify the queue connection on static:warm command #8634

Merged
merged 16 commits into from
Jul 16, 2024

Conversation

grantholle
Copy link
Contributor

@grantholle grantholle commented Aug 28, 2023

This is adding another workaround for #3291. It seems like the default queue connection needs to be sync, but when I deploy changes I'd like to warm using the queue. This change allows you to specify the connection using the --queue option.

I added an additional test, even though they are marked as incomplete at the moment. I also updated another assertion to match the current output. In my experience the tests seems ok.

Closes statamic/ideas#1153.

@grantholle grantholle changed the title Add ability to specify the queue connection on static:warm command [4.x] Add ability to specify the queue connection on static:warm command Aug 28, 2023
@jasonvarga
Copy link
Member

Thank you. I made the tests run. The comment said If you spam it_warms_the_static_cache, it'll eventually fail. ... I spammed as hard I could and it passed every time. We'll see if it works on GitHub.

@grantholle
Copy link
Contributor Author

Yeah I had the same experience ¯_(ツ)_/¯

@jasonvarga
Copy link
Member

I want to be sure that we make the difference between the queue connection and queue itself very obvious.

i.e. When you do please static:warm --queue=foo should that put the jobs on foo queue or use the foo connection?

I'm not sure which is the right option. Both could be argued, so we should find some other examples somewhere and follow convention.

@grantholle
Copy link
Contributor Author

Yeah I kind of had the same question. I did it this way since there's the statamic.static_caching.warm_queue config option. I can also see the merit of the config option having the queue connection in order to be consistent with statamic.git.queue_connection.

When looking at the queue commands in Laravel, the connection is always the argument, and --queue is the name of the queue. Which is inconsistent with what I have done here.

Maybe make the --queue option here be the name of the queue, and add a config option for statamic.static_caching.queue_connection?

@grantholle
Copy link
Contributor Author

Hey @jasonvarga I made some improvements to hopefully make it more clear to differentiate between queue name and connection. Let me know, thanks.

@grantholle
Copy link
Contributor Author

(I'll also fix the tests, just wondering if you think it's all right)

@grantholle
Copy link
Contributor Author

Hey @jasonvarga do you mind revisiting this?

Copy link
Member

@duncanmcclean duncanmcclean left a comment

Choose a reason for hiding this comment

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

This seems to be working well for me. Just had two thoughts around the new config option.

|
*/

'queue_connection' => env('STATAMIC_STATIC_WARM_QUEUE_CONNECTION'),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it might be worth changing this to warm_queue_connection to stay consistent with warm_queue above?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I wonder if it might be worth falling back to the default connection here so we don't need to do that in the StaticWarm command:

Suggested change
'queue_connection' => env('STATAMIC_STATIC_WARM_QUEUE_CONNECTION'),
'warm_queue_connection' => env('STATAMIC_STATIC_WARM_QUEUE_CONNECTION', config('queue.default')),

@duncanmcclean duncanmcclean changed the title [4.x] Add ability to specify the queue connection on static:warm command [5.x] Add ability to specify the queue connection on static:warm command May 13, 2024
@duncanmcclean duncanmcclean changed the base branch from 4.x to 5.x May 13, 2024 12:17
@jasonvarga
Copy link
Member

I've updated this so that --queue remains a simple boolean on the command. There is no --connection argument.

If you use --queue, it'll refer to your config to figure out which queue and connection to use.

@jasonvarga jasonvarga merged commit 54f54e6 into statamic:5.x Jul 16, 2024
16 checks passed
duncanmcclean added a commit to statamic/statamic that referenced this pull request Aug 7, 2024
duncanmcclean added a commit to statamic/statamic that referenced this pull request Aug 7, 2024
* Allow configuring the Stache's Cache Store

Related: statamic/cms#10303

* Ability to disable CP authentication

Related: statamic/cms#8960

* Display custom logo as plain text

Related: statamic/cms#10350

* Track sites.yaml path in git integration config

Related: statamic/cms#10463

* Add ability to specify the queue connection on static:warm command

Related: statamic/cms#8634
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add config option to set a custom queue
3 participants