-
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
Remove hardcoded AWS bucket Name from a Few Files #5799
Conversation
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.
👍 👍
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.
What a tedious tasks, thanks for doing this @mstruve! I know you said you're going to do views in a later PR, but the static public/offline.html
could potentially also go in this one.
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.
Hi @mstruve, thanks a lot for surfacing this but I don't think this is the right approach to fix the issue.
I believe we should download these images and add them to app/assets/images
with the others, so that we don't have hardcoded paths pointing to dying buckets or non harcoded paths pointing to potentially non existing images
@@ -5,7 +5,7 @@ class ProfileImage | |||
def initialize(resource) | |||
@resource = resource | |||
@image_link = resource.profile_image_url | |||
@backup_link = "https://thepracticaldev.s3.amazonaws.com/i/99mvlsfu5tfj9m7ku25d.png" | |||
@backup_link = "https://#{ApplicationConfig['AWS_BUCKET_NAME']}.s3.amazonaws.com/i/99mvlsfu5tfj9m7ku25d.png" |
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.
wouldn't this break local installations when merged?
For example:
- the local
AWS_BUCKET_NAME
is notthepracticaldev
by default, it'sOptional
- mine is called
devto-tests
because I configured it to point to my AWS installation - this would mean that I'd have to manually upload a file named
99mvlsfu5tfj9m7ku25d.png
for this to work
I think it'd have an impact on the other communities as well.
Maybe we could keep this hardcoded until we find a better way to handle this:
- either we download these images and put them in our assets (my fav option)
- or we find a way to upload them when the app is setup, through a script or something
Basically I don't think these images should be in AWS at all :)
@@ -2,7 +2,7 @@ module HtmlCssToImage | |||
AUTH = { username: ApplicationConfig["HCTI_API_USER_ID"], | |||
password: ApplicationConfig["HCTI_API_KEY"] }.freeze | |||
|
|||
FALLBACK_IMAGE = "https://thepracticaldev.s3.amazonaws.com/i/g355ol6qsrg0j2mhngz9.png".freeze | |||
FALLBACK_IMAGE = "https://#{ApplicationConfig['AWS_BUCKET_NAME']}.s3.amazonaws.com/i/g355ol6qsrg0j2mhngz9.png".freeze |
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.
same as the previous observation
@@ -14,7 +14,7 @@ class SiteConfig < RailsSettings::Base | |||
field :social_networks_handle, type: :string, default: "thepracticaldev" | |||
|
|||
# images | |||
field :main_social_image, type: :string, default: "https://thepracticaldev.s3.amazonaws.com/i/6hqmcjaxbgbon8ydw93z.png" | |||
field :main_social_image, type: :string, default: "https://#{ApplicationConfig['AWS_BUCKET_NAME']}.s3.amazonaws.com/i/6hqmcjaxbgbon8ydw93z.png" |
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.
he're we're definitely breaking the main social image for a pristine installation
@@ -46,7 +46,7 @@ def remove_profile_info | |||
twitch_url: nil, feed_url: nil | |||
) | |||
|
|||
user.update_columns(profile_image: "https://thepracticaldev.s3.amazonaws.com/i/99mvlsfu5tfj9m7ku25d.png") | |||
user.update_columns(profile_image: "https://#{ApplicationConfig['AWS_BUCKET_NAME']}.s3.amazonaws.com/i/99mvlsfu5tfj9m7ku25d.png") |
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.
this is further evidence we might want to rethink this approach, we are storing a broken path in the DB:
- it's either a broken URL for new installers
- or a URL that points to non existent images if the bucket name is not
thepracticaldev
@rhymes you are right, a lot of these images should be downloaded and put in the assets folder. Since there is a bit more scope to this than I anticipated I am going to close and make an issue to get it sorted out. |
@mstruve went ahead and put them all haahaahha |
What type of PR is this? (check all applicable)
Description
This is only a small number of the places we have hardcoded our old AWS bucket name. I figured I would do these and then make an Issue to handle all of the ones that are in the view and emails which are A LOT and will need some closer attention as some are cloudinary links and some point straight to S3.
Related Ticket
Followup for #5771 and #5555
Added to documentation?