huboard / huboard-web

GitHub issues made awesome
https://huboard.com
61 stars 26 forks source link

Implements a github repository image uploader storage class for carrierwave #325

Closed rauhryan closed 8 years ago

rauhryan commented 8 years ago

Closes huboard/huboard#520 when merged

Todo:

Later:

dahlbyk commented 8 years ago

So if you want to pull down the uploads ref, update your remote config with a new fetch spec:

[remote "origin"]
        url = https://github.com/rauhryan/skipping.stones.git
        fetch = +refs/heads/*:refs/remotes/origin/*
        fetch = +refs/huboard/*:refs/remotes/origin/hb/*
dahlbyk commented 8 years ago

To fill in some details from offline discussion...

  • add :org/:repo support for the uploader
  • Change both the upload end-points to POST

The idea here is to replace get 'uploads/asset' with a route like post /:user/:repo/upload that will return appropriate upload "instructions". For AwsUploader, the SaaS behavior, the instructions allow POSTing directly to AWS. For the new GitHub uploader, we need ApiUploader#direct_fog_url to return /:user/:repo/upload/create or equivalent so we can upload into the correct repository.

  • make sure we only allow fast forward commits

force: false is the default ref update behavior, so really we just need to make sure we can retry if the PATCH is rejected.

  • optimize upload paths with the folder structure convention

See https://github.com/huboard/huboard-web/pull/325#discussion_r67751456

  • reach out to enterprise users for how to handle no push bit (fork or fall-back user)

The downside of a fork is that the assets would be deleted if the fork is deleted. I really don't like the risk of data loss. My preference would be to simply disallow read-only user uploads unless a fall-back user token has been specified in the appliance configuration.

rauhryan commented 8 years ago

@discorick @dahlbyk I'm pretty sure I've addresses all the concerns, can we merge this?

dahlbyk commented 8 years ago

I'm pretty sure I've addresses all the concerns, can we merge this?

Sorry, haven't taken the time to review. Will try to do that tonight.