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

Allow for Configurable Archive Period #39

Merged
merged 3 commits into from
Mar 2, 2020
Merged

Conversation

tybritten
Copy link
Contributor

For #38. Adds a config item called ArchiveLength that defaults to 90 days.
Tests are updated, but still need to add tests specifically for changing the period

@tybritten tybritten changed the title [WIP] Allow for Configurable Archive Period Allow for Configurable Archive Period Jan 8, 2020
@tybritten
Copy link
Contributor Author

Ok, added tests for ArchiveLength

@tybritten
Copy link
Contributor Author

We're gonna run this on our side first, but I'd love some eyes on the changes I made to see if anything is wrong

Copy link
Contributor

@nicpottier nicpottier left a comment

Choose a reason for hiding this comment

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

Forgot we had thought about making that an org level item one day, so that was easy!

Do let us know how it works out.

@tybritten
Copy link
Contributor Author

so far, so good. It seems to be handling in-use objects gracefully level=warning msg="unable to delete broadcast, has messages still" broadcast_id=15552384 msg_count=1 org_id=1

@tybritten
Copy link
Contributor Author

Ok it completed all its runs and it worked exactly as expected. We used a period of 30 days and it worked fine.

@tybritten
Copy link
Contributor Author

Just an update- still working fine as expected

@nicpottier
Copy link
Contributor

Great, thanks, we'll merge in.

@nicpottier nicpottier merged commit df17765 into nyaruka:master Mar 2, 2020
@nicpottier
Copy link
Contributor

Actually just realized that maybe ArchiveLength may not be the best name for this. How do you feel about having that be named RetentionPeriod which feels more right? (I can make the change)

@tybritten
Copy link
Contributor Author

I agree with that.

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