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.

Sign up for GitHub

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

Jump to bottom

fix #1065: re-instating purge unused s3 assets cronjob #1134

Merged
merged 4 commits into from
May 5, 2020
Merged

fix #1065: re-instating purge unused s3 assets cronjob #1134

merged 4 commits into from
May 5, 2020

Conversation

phmngocnghia
Copy link
Contributor

@github-actions github-actions bot added aml-checklist Relates to Anti-money laundering app geo-diary Relates to GEO Diary app lifetime-legal Relates to Lifetime Legal App marketplace Relates to the Marketplace smb-onboarder Relates to Small Medium Business onboarding app labels May 4, 2020
willmcvay
willmcvay reviewed May 4, 2020
View reviewed changes
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

Looks fine, did you address the errors from the service worker in Sentry? Also, the toast should dismiss on re-load - I added a comment to the ticket

willmcvay
willmcvay approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

Approving as per chat at standup 👍

@phmngocnghia phmngocnghia changed the title [WIP] fix #1065: re-instating purge unused s3 assets cronjob fix #1065: re-instating purge unused s3 assets cronjob May 5, 2020
@phmngocnghia phmngocnghia force-pushed the fix-#1065-re-instating-purge-unused-s3-assets-cronjob branch from 1b6bc68 to 563ba2e Compare May 5, 2020 06:08
@github-actions github-actions bot requested a review from willmcvay May 5, 2020 06:09
@github-actions github-actions bot added the react-app-scaffolder Relates to React App Scaffolder package label May 5, 2020
@phmngocnghia phmngocnghia force-pushed the fix-#1065-re-instating-purge-unused-s3-assets-cronjob branch from 7326a5a to ed7ed3e Compare May 5, 2020 06:20
willmcvay
willmcvay reviewed May 5, 2020
View reviewed changes
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

Just a general comment, feels like there is a "jest" way to mock window.location

packages/geo-diary/src/core/__tests__/app.tsx Show resolved Hide resolved
duong-se
duong-se suggested changes May 5, 2020
View reviewed changes
Copy link
Contributor

@duong-se duong-se left a comment

Choose a reason for hiding this comment

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

Pls rebase this branch

@phmngocnghia phmngocnghia force-pushed the fix-#1065-re-instating-purge-unused-s3-assets-cronjob branch from 6798bc9 to 4c22383 Compare May 5, 2020 11:33
@github-actions github-actions bot requested review from duong-se and willmcvay May 5, 2020 11:33
duong-se
duong-se approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@duong-se duong-se left a comment

Choose a reason for hiding this comment

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

LGTM

@duong-se duong-se merged commit 6cb9322 into master May 5, 2020
@duong-se duong-se deleted the fix-#1065-re-instating-purge-unused-s3-assets-cronjob branch May 5, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duong-se duong-se duong-se approved these changes

@ant066 ant066 Awaiting requested review from ant066

@nphivu414 nphivu414 Awaiting requested review from nphivu414

@trankhacvy trankhacvy Awaiting requested review from trankhacvy

@vuhuucuong vuhuucuong Awaiting requested review from vuhuucuong

@willmcvay willmcvay Awaiting requested review from willmcvay

Assignees

@phmngocnghia phmngocnghia

Labels
aml-checklist Relates to Anti-money laundering app geo-diary Relates to GEO Diary app lifetime-legal Relates to Lifetime Legal App marketplace Relates to the Marketplace react-app-scaffolder Relates to React App Scaffolder package smb-onboarder Relates to Small Medium Business onboarding app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create CRON to purge S3 buckets based on commit hash
3 participants
@phmngocnghia @willmcvay @duong-se