-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[do not merge] Remove hardcoded AWS image urls and download images into assets #5829
[do not merge] Remove hardcoded AWS image urls and download images into assets #5829
Conversation
ea991f5
to
d4ba5ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@rhymes In my PR, I simply used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d4ba5ff
to
918e9d4
Compare
918e9d4
to
2c49f45
Compare
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 |
@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. |
@nickytonline it's builtin, it is mentioned in the initial description by @seb1441:
|
@seb1441, looks good for the |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
2c49f45
to
c200673
Compare
Hi @seb1441, the file |
6f7675a
to
6988e8c
Compare
Hi @rhymes, no problem. Done! |
The above is the only question I have left. From what I can tell, on my local, t seems to just act like |
@nickytonline Sorry I skipped your question before. The reason But on prod it will work. Example, this: If you wanna try it, you can set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last small change and this is good to go.
6988e8c
to
f278f0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
enhance_image_tag: true | ||
static_file_support: false | ||
|
||
production: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benhalpern can you explain further what you mean here since it seems we want to get this pushed out
Just commenting on here to re-surface this PR. What do we still need, what are the next steps? |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not super familiar with Cloudinary so I would like to know more about @benhalpern's concerns with this new implementation.
enhance_image_tag: true | ||
static_file_support: false | ||
|
||
production: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benhalpern can you explain further what you mean here since it seems we want to get this pushed out
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:
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) |
Hi @rhymes, I understand. No problem, you can "adopt" my PR. |
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!!!! |
What type of PR is this? (check all applicable)
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:
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.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.pngapi_v0.yml
,offline.html
andREADME.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?