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

Non destructive sync option (keep-old) #91

Merged
merged 8 commits into from
Aug 1, 2023

Conversation

keithbrink
Copy link
Contributor

@keithbrink keithbrink commented Jul 28, 2023

I had an issue recently where I created a cron job I wanted to monitor on the Oh Dear dashboard, and it was deleted the next time I synced my schedule. I created a separate PR to document to call out this behaviour (#90), but it would not be needed if this PR is merged.

This PR adds a --keep-old option to the sync command that will only create new monitors on Oh Dear, rather than a destructive sync. This has the downside of not removing monitors for jobs that no longer exist in the schedule or updating changes to existing monitors, but allows you to get 90% of the benefits of syncing while still being able to monitor cron jobs not managed by Laravel.

There is the possibility of adding support for updating existing monitors by matching the ping url, but interested to get feedback on this first.

I had some issues with the tests not creating snapshots properly, not too sure what the issue was there but I added the pest snapshot plugin as I believe that was the issue; let me know if I'm missing something.

@keithbrink
Copy link
Contributor Author

I see #90 was merged while I was writing this :)

@keithbrink
Copy link
Contributor Author

Had another look at the snapshot tests and it looks like that test wasn't working properly (see previous action results) after the testsuite was converted to Pest, so if this MR is not merged, I will create a separate MR to fix that test.

image

Copy link
Member

@freekmurze freekmurze 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 work on this!

src/Commands/SyncCommand.php Outdated Show resolved Hide resolved
src/Commands/SyncCommand.php Outdated Show resolved Hide resolved
src/Commands/SyncCommand.php Outdated Show resolved Hide resolved
@keithbrink
Copy link
Contributor Author

@freekmurze Updates made as requested

@keithbrink keithbrink changed the title Non destructive sync option (push) Non destructive sync option (keep-old) Jul 31, 2023
@freekmurze freekmurze merged commit cde159b into spatie:main Aug 1, 2023
@freekmurze
Copy link
Member

Thanks!

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.

2 participants