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

added an optional cronjob to perform the dbclean (external trigger) #52

Merged

Conversation

sherifkayad
Copy link
Contributor

fixes #51

Implementing the external DB Clean Task recommended as per the docs https://docs.pact.io/pact_broker/docker_images/pactfoundation#running-the-clean-task-from-an-external-source

  • Got rid of the DB Clean inside the Deployment
  • Utilized a CronJob to perform the cleanup
  • Generalized logging & db env vars (included them in the helm helpers file) as they are now re-usable across the CronJob & the Deployment

@sherifkayad
Copy link
Contributor Author

@ChrisJBurns would be great if you can have a look at this one 😉

@ChrisJBurns
Copy link
Contributor

Thanks for this @sherifkayad I'll give it a review when I've got a bit more time, I'll have to make sure that it all templates as expected as well

@sherifkayad
Copy link
Contributor Author

@ChrisJBurns TYT and let me know if anything needs to be changed

@ChrisJBurns
Copy link
Contributor

@sherifkayad Just had a look at this, we should probably make the CronJob optional for the database clean just to keep things simple in the vanilla install. So by default, if someone installs a single Pact Broker with 1 replica, then that single instance can be the container that runs the clean. But for folks who have more of a replicated setup of the Pact Brokers, we can allow them to enable the DBClean CronJob, a bit of documentation in the README.md about this would be a good thing too, just so users are aware of the ways the DB Clean can be done 👍

So a few tweaks, but all round great addition! 👍

@sherifkayad
Copy link
Contributor Author

@sherifkayad thanks for the feedback. Let me take care of that and submit something you can have a look at. Will ping you once I add something there.

@sherifkayad sherifkayad changed the title used a cronjob to perform the dbclean added an optional cronjob to perform the dbclean (external trigger) Apr 2, 2023
@sherifkayad
Copy link
Contributor Author

@ChrisJBurns can you please have a look now? I also added some docs in the README about that new feature and overall about the DB Cleanup Task.

Signed-off-by: Sherif Ayad <sherif.k.ayad@gmail.com>
@sherifkayad
Copy link
Contributor Author

@ChrisJBurns any updates from your side to the latest changes done?

@ChrisJBurns
Copy link
Contributor

@sherifkayad Yep, I plan on taking a look on Friday as it's bank holiday here in the UK 👍

@sherifkayad
Copy link
Contributor Author

Awesome! Same here in Germany .. Happy Easter holidays and let me know if there is anything I can do

Copy link
Contributor

@ChrisJBurns ChrisJBurns left a comment

Choose a reason for hiding this comment

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

/lgtm

@ChrisJBurns ChrisJBurns merged commit 37a7833 into pact-foundation:master Apr 7, 2023
@sherifkayad sherifkayad deleted the external-db-clean-job branch April 7, 2023 15:38
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.

Pact Broker External DB Clean Task
2 participants