[do not merge] Remove hardcoded AWS image urls and download images into assets

[websebdev]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

I removed hardcoded AWS image urls and downloaded them in the assets folder with appropriate names.
As a bonus, all the broken image urls/displaying in local that I noticed seem to be fixed.

Notes:

• For cloudinary image urls, I used cl_image helper.
  IMPORTANT: However, we will need to run rake cloudinary:sync_static to upload local images to cloudinary and push the generated files .cloudinary.static and .cloudinary.static.trash, which keeps track of what's been uploaded, in git. I did not include my files because I believe it's linked to my cloudinary account so it needs to be done with DEV's cloudinary account.
• There was one image, octopus.png which was too big for cloudinary (12.3 MB) so I had to reduce its size. https://thepracticaldev.s3.amazonaws.com/i/jl3vwgqxcx37vr45ln2o.png
• There are still AWS image urls left in api_v0.yml, offline.html and README.md but I assume that we need to leave them like that.

Related Tickets & Documents

Closes #5810

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

[rhymes]

Thanks for reducing octopus.jpg size!

I'm linking the documentation that explains rake cloudinary:sync_static to let Cloudinary serve the embedded static assets: https://cloudinary.com/documentation/rails_image_manipulation#static_files

I was wondering if it'd be easier either to use cloudinary for all static assets in this PR (now some use image_tag and some use cl_image_tag) and to use it for none, what's the criteria?

[websebdev]

@rhymes In my PR, I simply used cl_image_tag when the image url was using cloudinary and image_tag if not. But the criteria could be to use cl_image_tag when we want to do any image manipulation/transformation with cloudinary, otherwise image_tag as it's probably a bit more performant.

[rhymes]

I think the criteria between cl_image_tag and image_tag makes sense.

Thanks for doing this massive undertaking @seb1441, really appreciated!

NOTE for reviewers: this PR needs to be paired with a script that uploads assets to Cloudinary. Please check the issue description.

[rhymes]

Hi @thepracticaldev/sre - I tagged you for the review because this PR requires uploading static images to our Cloudinary account, it's not just about the code itself

[nickytonline]

@rhymes, do we have a script to upload to Cloudinary? I looked in the issue and there does not appear to be one linked in the issue.

[rhymes]

@nickytonline it's builtin, it is mentioned in the initial description by @seb1441:

IMPORTANT: However, we will need to run rake cloudinary:sync_static to upload local images to cloudinary and push the generated files .cloudinary.static and .cloudinary.static.trash, which keeps track of what's been uploaded, in git. I did not include my files because I believe it's linked to my cloudinary account so it needs to be done with DEV's cloudinary account.

[nickytonline]

@seb1441, looks good for the image_tag conversions. I'm testing cl_image_tag. but just sorting out some issues when running rake cloudinary:sync_static. I'm getting CloudinaryException: Invalid Signature xxx. String to sign - xxxx. I've never used Cloudinary, so just reading through their docs. Figured it out

[nickytonline]

@seb1441, I also noticed that the about page is different in your branch. The latest in production only has a group picture of the founders and then a team photo below it. You'll need to update your branch with the latest from the main repository's master.

[nickytonline]

@seb1441, I couldn't find it in the documentation, but when cl_image_tag is used in development mode, it seems to generate URL like image_tag does. Is this the expected behaviour? If so, the PR looks good. As mentioned, just update the branch with the latest from the main repository's master and this should be good to go.

[rhymes]

Hi @seb1441, the file app/views/podcast_episodes/_subscribe_buttons.html.erb disappeared from master recently so you might have to rebase again and remove it from the conflict, sorry!

[websebdev]

Hi @rhymes, no problem. Done!

[nickytonline]

@seb1441, I couldn't find it in the documentation, but when cl_image_tag is used in development mode, it seems to generate URL like image_tag does. Is this the expected behaviour? If so, the PR looks good. As mentioned, just update the branch with the latest from the main repository's master and this should be good to go.

The above is the only question I have left. From what I can tell, on my local, t seems to just act like image_tag. Is that the expected behaviour, i.e. no Cloudinary URL on my local?

[websebdev]

@nickytonline Sorry I skipped your question before.

The reason cl_image_tag acts as a normal image_tag (as in, the generated url does not point to cloudinary) is because I set static_file_support to false in development and test environments. That way, anyone will be able to see images even if they did not set their cloudinary credentials and ran the cloudinary rake task.

But on prod it will work. Example, this:
cl_image_tag("jess-lee.jpg", width: 250, crop: :scale, fetch_format: "auto", flags: ["progressive"], quality: "auto", type: :asset, alt: "Jess Lee")
will output
<img width="250" alt="Jess Lee" src="https://res.cloudinary.com/dwpdeql2w/image/asset/c_scale,f_auto,fl_progressive,q_auto,w_250/jess-lee-a3a8411dc19b6efbfeae56c55a6d6445.jpg">

If you wanna try it, you can set static_file_support to true for development in config/cloudinary.yml.

[nickytonline]

One last small change and this is good to go.

[nickytonline]

LGTM. As @seb1441 mentions in the description, whoever deploys this will need to run the rake cloudinary:sync_static and commit those files that get generated as part of this PR. cc: @rhymes @mstruve

[rhymes]

Thanks for this, we need help from the SRE team to sign off on this and perform the required steps for the deployment but code wise it's fine!

Great job @seb1441!

[benhalpern]

At first glance, this is what could possibly be inconsistent with what we currently do? I'm not sure, just an inkling.

I can dive deeper later.

[mstruve]

@benhalpern can you explain further what you mean here since it seems we want to get this pushed out

[citizen428]

Just commenting on here to re-surface this PR. What do we still need, what are the next steps?

[rhymes]

@citizen428 I think we need @thepracticaldev/sre team's to do a review of this, making sure there's nothing concerning and we also need their help in deploying it.

[mstruve]

I am not super familiar with Cloudinary so I would like to know more about @benhalpern's concerns with this new implementation.

[mstruve]

@benhalpern can you explain further what you mean here since it seems we want to get this pushed out

[rhymes]

Hi @seb1441, sorry for the late response.

We've discussed this PR internally and we'd like to revive it and take it to completion, also in lieu to our generalization efforts as you well know.

The main blocker (aside from conflicts due to our delayed response time) is that we don't plan to upload images to cloudinary. The flow for our website should always be:

1. add the images to the repo
2. tell cloudinary to fetch them and optimize them
3. show them to the user

Thus, we can't "sync" them to Cloudinary ahead of time.

If it's alright with you we'd like to "adopt" the PR and bring it to completion. Let us know what you think.

(cc. @Ridhwana which will likely be the team member working on finalizing this)

[websebdev]

Hi @rhymes, I understand. No problem, you can "adopt" my PR.

[mstruve]

Since this is going to be brought in house for completion I am going to close this until it can be updated and is ready to merge.

Thanks @seb1441 for all your hard work on this!!!!