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

Cleanups and Fixes for Emergency Access #2936

Merged
merged 1 commit into from Dec 4, 2022

Conversation

BlackDex
Copy link
Collaborator

@BlackDex BlackDex commented Nov 26, 2022

  • Several cleanups and code optimizations for Emergency Access
  • Fixed a race-condition regarding jobs for Emergency Access
  • Some other small changes like allow(clippy::) removals

Fixes #2925
Fixes #2955

@BlackDex BlackDex force-pushed the emergency-access-cleanup branch 3 times, most recently from 18a5346 to e885064 Compare December 1, 2022 14:24
Copy link
Contributor

@tessus tessus left a comment

Choose a reason for hiding this comment

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

I checked the other cron jobs as well and all of them are wrong.

Cron format:

*    *    *    *    *  Command_to_execute
|    |    |    |    |       
|    |    |    |    Day of the Week ( 0 - 6 ) ( Sunday = 0 )
|    |    |    |
|    |    |    Month ( 1 - 12 )
|    |    |
|    |    Day of Month ( 1 - 31 )
|    |
|    Hour ( 0 - 23 )
|
Min ( 0 - 59 )

According to @stefan0xC, the format is different than the standard cron format.

I'll open a PR to add this info to the template file. This is certainly not clear by just looking at the file....

Update: Scratch that too. It is in the .env.template file. Strange that I missed this.

.env.template Show resolved Hide resolved
- Several cleanups and code optimizations for Emergency Access
- Fixed a race-condition regarding jobs for Emergency Access
- Some other small changes like `allow(clippy::)` removals

Fixes dani-garcia#2925
@BlackDex
Copy link
Collaborator Author

BlackDex commented Dec 2, 2022

Update: Scratch that too. It is in the .env.template file. Strange that I missed this.

That was in one of my updates, so it could be you didn't see a recent version.
Because of the difference between normal cron's and sometimes the complexity i thought to add some more info about it.

The seconds option is btw a nice addition, at least for me during testing, instead of having to wait a whole minute or 30 seconds hehe. But for that to work, you also need to change the JOB_POLL_INTERVAL_MS to a lower amount, say 500 (half a second), else it will not work, because the default is 30s

@tessus
Copy link
Contributor

tessus commented Dec 2, 2022

That was in one of my updates, so it could be you didn't see a recent version.

Yep, this is possible, because I remember I read the entire template file a while back and that part was new to me.

Because of the difference between normal cron's and sometimes the complexity i thought to add some more info about it.

Good idea. It definitely helps.

The seconds option is btw a nice addition, at least for me during testing,

Yep, I totally agree. Personally I would have just added new fields at the end, so that it is still compatible with the original format. But I also understand why the dev of the library wanted to have the seconds next to the minutes...

@dani-garcia dani-garcia merged commit 6bbb3d5 into dani-garcia:main Dec 4, 2022
@BlackDex BlackDex deleted the emergency-access-cleanup branch December 5, 2022 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants