-
Notifications
You must be signed in to change notification settings - Fork 95
fix(docker): Add default crontab download command #1321
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
base: master
Are you sure you want to change the base?
Conversation
|
Also clarifies the Automating page in the Getting Started guide docs. |
51189d9 to
d9a57c0
Compare
|
Should be merged after #1318. It's not based on it and I'm not aware of any conflicts, but for max narrative consistency, it would be better the merge the earlier one first. |
d9a57c0 to
b60dda8
Compare
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
2287c78 to
7998815
Compare
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
From [PR feedback](jmbannon#1321 (comment)).
4baef1f to
fc6727a
Compare
| # create cron script wrapper | ||
| echo '#!/bin/bash' > "$CRON_WRAPPER_SCRIPT" | ||
| # Echo commands for easier user debugging: | ||
| echo "set -x" >> "$CRON_WRAPPER_SCRIPT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this is helpful for us devs, I dont think the user should see this. Let's remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that for users to help them debug their cron script. The user's script is outside the image's control but the wrapper is under it's control and there are user errors where echoed commands could help them understand their mistake. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think users adding their own echo's are sufficient. Seeing echo command followed by the actual echo output seems odd for default behavior with no way to disable
| will run *whenever* the container starts including when the host reboots, when ``# | ||
| dockerd`` is upgraded, when a new image is pulled, when something applies Compose | ||
| changes, etc.. This may result in running ``ytdl-sub`` right before or after the next | ||
| cron scheduled run or even at the same time on top of each other. Instead, run your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ytdl-sub can't run in parallel via file lock checks. Lets omit this wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I made a mental note to ask about that at the time. I should not rely on mental notes. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, where is that behavior documented, @jmbannon ? I don't recall coming across it. Happy to find a good place to clarify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sort of hidden from the user, but can kinda be found here: https://ytdl-sub.readthedocs.io/en/latest/config_reference/config_yaml.html#ytdl_sub.config.config_validator.ConfigOptions.lock_directory
| services: | ||
| ytdl-sub: | ||
| environment: | ||
| CRON_SCHEDULE: "0 */6 * * *" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets also have cron run on start here with it set to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, as a throttle footgun, consider a user who copies this snippet to their Compose config and then after running successfully for months wants to download outside the schedule. We want that user to come back to the docs and read the warning, but if they have that in their config it encourages them to just change it to true and they miss the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im still not convinced its as bad as you think, throttle protection ideally will mitigate most of the issue. Worst case is they skip over their subscription sleep. The other flag already gives them control to start it every 1m so I say give them the rope 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im still not convinced its as bad as you think
I was thinking about that after my previous comment, if this is an issue for users it's unlikely to be attributed to this env var. At best, a user will notice they've been throttled during the run triggered by the container start. At worst, they're getting throttled daily when their container is restarted for any of the reasons above, but downloads still happen before a restart happened too close to a scheduled run, so all they notice is "sluggish" downloads of new entries. New entries sometimes download "on-time" but sometimes days "late". Either way, they will approach it as a throttle issue and almost never notice the correlation to container restarts.
throttle protection ideally will mitigate most of the issue. Worst case is they skip over their subscription sleep.
My experience with triggering throttles, and not just resolution throttles, and bans indicates the opposite. My efforts to tune throttle protection have had limited success and even when I was most successful it was extremely sensitive to how frequently I re-ran. That was the other reason I was setting alarms for the end of the sleep_per_download_s: delay, I triggered throttles often when I short cut the download delay even by just 1/3-1/4.
The other flag already gives them control to start it every 1m so I say give them the rope 😅
There's a big difference when guiding new users, and this is the Getting Started guide, between a feature with a safe default that a user can abuse if they figure out how, and a toggle feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still under the impression it can be toggled in a safe manner. The current representation of it commented out seems like it's a legacy do-not-use feature when it has been commonly requested and used (#730).
I understand the sentiment to protect new users, and I think the warning message below does a good job of that (maybe an addition to the FAQ would too). My friendly ask is to keep the yaml as is:
environment:
CRON_SCHEDULE: "0 */6 * * *"
CRON_RUN_ON_START: false
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
From [PR feedback](jmbannon#1321 (comment)).
fc6727a to
47bfdb0
Compare
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
From [PR feedback](jmbannon#1321 (comment)).
47bfdb0 to
fde4cab
Compare
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
From [PR feedback](jmbannon#1321 (comment)).
fde4cab to
d90c0eb
Compare
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
From [PR feedback](jmbannon#1321 (comment)).
d90c0eb to
e1c354b
Compare
Clarify the `/config/ytdl-sub-configs/cron` script with explanatory comments and a default `--dry-run` command. Also change the wrapper script to echo commands for easier debugging. To update an existing script, move the old script aside, restart the container to regenerate it, and edit the new script. Also clarify the Automating page in the Getting Started guide docs.
[PR feedback](jmbannon#1321 (comment)) prompted me to reconsider having a default command at all. We should assume, unfortunately, that many new users will just skim the docs enough to enable the image's cron integration but not actually incrementally test their configuration. In those cases, they'd end up sending dry-run non-download requests for all their subscriptions every 6 hours for no good reason. There's just no way to provide a default command that isn't also providing a footgun.
From [PR feedback](jmbannon#1321 (comment)), this seems like less of an ad than the previous and the source for the page is itself open source.
From [PR feedback](jmbannon#1321 (comment)).
Addressing this underlying issue requires more thought and should be a separate PR.
e1c354b to
9e7d6d3
Compare
Clarify the
/config/ytdl-sub-configs/cronscript with explanatory comments and a default--dry-runcommand. Also change the wrapper script to echo commands for easier debugging. To update an existing script, move the old script aside, restart the container to regenerate it, and edit the new script.