search

guardian / ophan-housekeeper

Lambda to remove Ophan Email Alerts for bouncing email addresses
0 stars 1 forks source link

Update Housekeeper code and Readme for local testing #264

Closed deedeeh closed 1 year ago

deedeeh commented 1 year ago

What does this change?

We upgraded Ophan Housekeeper from Scala 2 to Scala 3 #257 and that means the main function must be changed because it is not the same in Scala 3 and this is how we can run locally.

How to test

Run locally to see if the changed code works as expected. For more information check the updated Readme.

How can we measure success?

We are able to test locally.

Have we considered potential risks?

When running locally the developer creates an email alert on their own account so that shouldn't cause any alerts deletion except for the developer.

deedeeh commented 1 year ago

Looks good! There are a couple of minor superfluous changes, but definitely a big improvement!

What are the minor superfluous changes?

deedeeh commented 1 year ago

Just a tiny unnecessary change in AlertDeletion.scala, which makes the diff of this PR a little bigger than it needs to be! For me, it's always good to look at the 'Files changed' view and see how to minimise the size of the diff, removing unnecessary changes for reviews to review. This PR only really needs to change 2 files, but was initially changing 4, so there was a bit of potential for tidying up!

It can be a bit boring manually editing files back to how they were, but on the command line, you can use a command like this to get a file to back to how it was on the main branch:

git checkout main -- src/main/scala/housekeeper/AlertDeletion.scala

You have a point and I totally agree with it! Now we have 2 changed files and thank you for sharing that command which I didn't know about. I will merge those changes 🚀

frederickobrien commented 1 year ago

Great work! I tested locally using the README instructions and all went smoothly