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 optional email cleanup on S3 at end of process #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nbcraft
Copy link

@nbcraft nbcraft commented Jan 7, 2018

No description provided.

@nbcraft nbcraft force-pushed the nbcraft/feature/email-cleanup branch from 53348d8 to c32a6e1 Compare January 7, 2018 07:56
@jakubboucek
Copy link
Contributor

Looks good. But probably unnecessary, because you can set on bucket LifeCycle Management to expire object. For example: you can set to delete received ecah email after 1 day.

@nbcraft
Copy link
Author

nbcraft commented Jan 7, 2018

Yeah I saw that after developing the feature.. 😞
But one argument for it though is that it will delete only if the rest of the process went through, preventing a cleanup by the bucket LifeCycle Management when something went wrong and an email wasn't properly forwarded.
It's up to you guys 😉
EDIT: Also Travis crashed because I force pushed (I've seen that in the past), start it again if you please.

Copy link
Contributor

@jakubboucek jakubboucek left a comment

Choose a reason for hiding this comment

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

You move the steps below, and you push one item to it here. That broke extendability of this array by overrides.steps.

I suggest: revert changes in exports.handler method, just add exports.emailsCleanupOnS3 to array steps directly without conditions. And move evaluation of the data.config.emailsCleanupOnS3 inside to exports.emailsCleanupOnS3 function.

If you can, please update README.md documenation (especially paragraph 8) to complete your PR.

Please revert your change of package.json too. Versioning of releases should be privilege to @arithmetric when he will merge it.

TIP: just add new commit (no ammend&force-push), @arithmetric can squash it during merge.

@jakubboucek
Copy link
Contributor

jakubboucek commented Jan 7, 2018

Back to our discussion: you are right, but my experience is:

Set LifeCycle to 1 week at least. It is good for keep stack cleaned, but still ready to debug when some some is forwarded, but looks corrupted. When you want to erase is manually after success redirect, is appropriate to turn on versioning to able recovery deleted messages for unexpected fails and then must it clear it again by LifeCycle - OK, but it looks (for me) as the same concept, but more complex.

But you have right, deleting of bucket objects can be useful for some users.

Thanks you for contribution 👍

@nbcraft
Copy link
Author

nbcraft commented Jan 8, 2018

Updated PR from recommendations.

@astrocycles
Copy link

Hi,
I'm interested to ad a lifecycle to 1 week, but i don't know how to write this piece of code and where.
could you please help ?
many thanks
ps : great forward tool btw ! I'm using it everyday

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.

3 participants