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.

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

Fix/upload image url #317

Merged
merged 12 commits into from
Dec 27, 2021
Merged

Conversation

ayuki-joto
Copy link
Collaborator

🎩 What? Why?

uploadされた画像の配信をS3からcloudfrontで行うようにする

📌 Related Issues

https://meta.diycities.jp/assemblies/nazo/f/286/proposals/297

📋 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • If there's a new public field, add it to GraphQL API
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

Copy link
Contributor

@komtaki komtaki left a comment

Choose a reason for hiding this comment

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

Cloud Formationのymlのcommitもお願いします。

すでに本体でつかってるこれをいじれば簡単にできると思います。

@ayuki-joto
Copy link
Collaborator Author

@komtaki これでどうでしょうか?

@ayuki-joto ayuki-joto requested a review from komtaki December 19, 2021 18:02
Comment on lines 20 to 32
def url(*)
encoded_path = encode_path(path)
if (host = asset_host)
if host.respond_to? :call
"#{host.call(self)}/#{encoded_path}"
else
"#{host}/#{encoded_path}"
end
else
(base_path || "") + path
end
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

ちょっとここの定義がなぜこうなっているかがよく分かりませんでした…。
それ以外は特に問題なさそうでした。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これがいうゆるcloud frontのエンドポイントにs3の画像の宛先を向けている箇所で、中身に関しては、carriwaveからほとんどそのまま取ってきたので、aseet_hostが必ずある前提で組むでもいいかなとも思ってます!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

形修正しました!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Carrierwaveあんまり詳しくないのですが、たしかにこんな感じになるかなあ…という気もします(あんまり自信ないですが)。
とりあえずこれでやってみるでいいんではないでしょうか。

.cloudformation/cloud_front.yml Outdated Show resolved Hide resolved
deployments/.elasticbeanstalk/config.yml Outdated Show resolved Hide resolved
@komtaki
Copy link
Contributor

komtaki commented Dec 20, 2021

@komtaki これでどうでしょうか?

対応ありがとうございます。:bow:

@ayuki-joto
Copy link
Collaborator Author

@komtaki
s3にポリシー書く部分抜けてたので追加しました!
その際対象のs3のbucketを作っている部分がなかったので追加しましたが、こちら作らずに外部からbucket名だけ流すの方がいいでしょうか?

@komtaki
Copy link
Contributor

komtaki commented Dec 21, 2021

@komtaki s3にポリシー書く部分抜けてたので追加しました! その際対象のs3のbucketを作っている部分がなかったので追加しましたが、こちら作らずに外部からbucket名だけ流すの方がいいでしょうか?

ありがとうございます。s3を新しく作るなら、一緒にcloud formationで作る。作り直さないならパラメーターでアタッチするでいいと思います。:+1:
理想としてはcloud formationで作る方が再現性があっていいですね。

@ayuki-joto
Copy link
Collaborator Author

@komtaki

s3を新しく作るなら、一緒にcloud formationで作る。作り直さないならパラメーターでアタッチするでいいと思います。

ということはそこの分岐などを用意した方がいいんでしょうか..?
すみません. cloud formationでどこまでできるのかわかっていなくて。

.cloudformation/cloud_front.yml Outdated Show resolved Hide resolved
.cloudformation/cloud_front.yml Outdated Show resolved Hide resolved
.cloudformation/cloud_front.yml Outdated Show resolved Hide resolved
@komtaki
Copy link
Contributor

komtaki commented Dec 22, 2021

@komtaki

s3を新しく作るなら、一緒にcloud formationで作る。作り直さないならパラメーターでアタッチするでいいと思います。

ということはそこの分岐などを用意した方がいいんでしょうか..? すみません. cloud formationでどこまでできるのかわかっていなくて。

さすがに分岐はできないです。今回はs3のバケットを作るなら、今のままで不要なパラメータを削ればいいと思います。:+1:
#317 (comment)

「全部Cloud Formationで作るなら、yamlに全部書く」、「一部手動で作成したリソースをCloud Formationで使うなら、パラメータでARNとかを流し込んで使う」のどちらかの対応をするのかという意味です。

ayuki-joto and others added 2 commits December 23, 2021 13:40
Co-authored-by: Taki Komiyama <39375566+komtaki@users.noreply.github.com>
@ayuki-joto
Copy link
Collaborator Author

指摘箇所修正しました!
基本的には、「全部Cloud Formationで作るなら、yamlに全部書く」が良いかと思うのでその方向で対応しています!

@ayuki-joto ayuki-joto requested a review from komtaki December 24, 2021 05:51
Copy link
Contributor

@komtaki komtaki left a comment

Choose a reason for hiding this comment

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

LGTM 👍
いろいろ対応ありがとうございます。:clap:

@ayuki-joto ayuki-joto merged commit 7af3553 into codeforjapan:develop Dec 27, 2021
@ayuki-joto ayuki-joto mentioned this pull request Dec 28, 2021
6 tasks
@ayuki-joto ayuki-joto deleted the fix/upload-image-url branch April 25, 2022 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants