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

chore(platform): delete old images on upload of new ones #2738

Merged
merged 9 commits into from
Nov 14, 2023
Merged

chore(platform): delete old images on upload of new ones #2738

merged 9 commits into from
Nov 14, 2023

Conversation

Cosmin-Parvulescu
Copy link
Contributor

@Cosmin-Parvulescu Cosmin-Parvulescu commented Oct 23, 2023 •
edited
Loading

Description

  • Added delete endpoint to the image worker
  • Added 'change' checks to existing image url properties
  • Call delete endpoint after update (so it doesn't interfere with the actual updating)

Related Issues

Testing

  • Visual check in CF image bucket

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@Cosmin-Parvulescu Cosmin-Parvulescu added the enhancement Indicates new feature requests label Oct 23, 2023
@Cosmin-Parvulescu Cosmin-Parvulescu self-assigned this Oct 23, 2023
@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/platform/delete-old-images-on-upload-of-new-ones branch from 87cce46 to 3ce1cb1 Compare October 31, 2023 07:22
@Cosmin-Parvulescu Cosmin-Parvulescu marked this pull request as ready for review October 31, 2023 12:54
betimshahini
betimshahini requested changes Nov 1, 2023
View reviewed changes
apps/console/app/routes/apps/$clientId/auth.tsx Outdated
@@ -203,6 +208,8 @@ export const action: ActionFunction = getRollupReqFunctionErrorWrapper(
}

if (Object.keys(errors).length === 0) {
const ogAppIcon = appDetails.app?.icon
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the OAuth logo, not the OG icon. Rename the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, though appIcon feels closer to the app.icon than oauthLogo. Also OG stands for original, so originalAppIcon would have been the expanded version.

platform/images/src/jsonrpc/methods/delete.ts
const imageComponents = input.split('/')
const imageID = imageComponents[imageComponents.length - 2]

const url = `https://api.cloudflare.com/client/v4/accounts/${ctx.env.INTERNAL_CLOUDFLARE_ACCOUNT_ID}/images/v1/${imageID}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it the Image API with Custom ID couldn't be used to keep the image URL static and replace the image whenever it gets updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't consider that. I can do exploratory work on this if wanted.

@Cosmin-Parvulescu Cosmin-Parvulescu marked this pull request as draft November 2, 2023 10:14
@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/platform/delete-old-images-on-upload-of-new-ones branch 2 times, most recently from 28deb15 to b200a88 Compare November 9, 2023 10:11
betimshahini
betimshahini requested changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@betimshahini betimshahini left a comment

Choose a reason for hiding this comment

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

Code doesn't run ("params is undefined" error), likely due to recent changes that have landed on main.

apps/console/app/routes/apps/$clientId/auth.tsx Outdated
headers: generateTraceContextHeaders(context.traceSpan),
})

await imageClient.delete.mutate(oauthLogo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way to move this outside of the req-res lifecycle, through ctx.waitUntil()?

apps/profile/app/routes/account/profile.tsx Outdated
headers: generateTraceContextHeaders(context.traceSpan),
})

await imageClient.delete.mutate(ogImage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as other comment, to move out of req-res lifecycle.

apps/console/app/routes/apps/$clientId/auth.tsx Outdated
@@ -319,6 +336,29 @@ export default function AppDetailIndexPage() {
onChange={() => {
setIsFormChanged(true)
}}
onSubmitCapture={async (event) => {
event.preventDefault()
Copy link
Contributor

Choose a reason for hiding this comment

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

As this chunk of code would need to be reused in similar form for the other spots we upload images, would it make sense to refactor into a function that takes some params, like form and the loader effect functions, and takes care of the rest?

@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/platform/delete-old-images-on-upload-of-new-ones branch from b200a88 to 3188ba6 Compare November 10, 2023 10:23
@Cosmin-Parvulescu Cosmin-Parvulescu marked this pull request as ready for review November 14, 2023 11:15
@szkl szkl merged commit ef1fc0e into main Nov 14, 2023
14 checks passed
@szkl szkl deleted the chore/platform/delete-old-images-on-upload-of-new-ones branch November 14, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betimshahini betimshahini betimshahini approved these changes

@szkl szkl szkl approved these changes

Assignees

@Cosmin-Parvulescu Cosmin-Parvulescu

Labels
enhancement Indicates new feature requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore(platform): delete old images on upload of new ones
3 participants
@Cosmin-Parvulescu @szkl @betimshahini