Skip to content

Attach to release #673

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.

Sign up for GitHub

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

Jump to bottom
Merged
lunny merged 6 commits into from
Jan 15, 2017
Merged

Attach to release #673

lunny merged 6 commits into from
Jan 15, 2017

Conversation

@couling
Copy link
Contributor

@couling couling commented Jan 15, 2017 •
edited
Loading

Implemented attaching files to a release as requested in #282

There is already a field in the Attachment table which is unused not used called ReleaseID. This allows an attachment to be associated with a release. This PR:

  • Adds an upload box for users to upload attachments when creating / editing a release.
  • Associates uploaded attachments with releases using the existing ReleaseID field
  • Adds attachments as download links on the release page below source zip and tar links
  • Moves attachments POST URL from /issues/attachments to /attachments (the GET url was already /attachments)
  • Changes default upload file types to include zip and gzip files as these are the most likely files to attach to a release

appleboy
appleboy reviewed Jan 15, 2017
View reviewed changes
cmd/web.go Outdated
}
})
m.Post("/issues/attachments", repo.UploadIssueAttachment)
m.Post("/attachments", repo.UploadIssueAttachment)
Copy link
Member

@appleboy appleboy Jan 15, 2017

Choose a reason for hiding this comment

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

Why change this?

Copy link
Member

@appleboy appleboy Jan 15, 2017

Choose a reason for hiding this comment

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

change UploadIssueAttachment to UploadAttachment

Copy link
Member

@lunny lunny Jan 15, 2017

Choose a reason for hiding this comment

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

^

Copy link
Contributor Author

@couling couling Jan 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, will change the function name. The function repo.UploadIssueAttachment does nothing to do with Issues.

For future reference I changed this because I saw three options:

  • Have releases uploading attachments to /issues/attachments
  • add /releases/attachments and point to the same function or copy/paste function
  • rationalise it and change the existing URL to just /attachments

Given that the GET url is already /attachments the third option seemed the most rational.

@lunny lunny added this to the 1.1.0 milestone Jan 15, 2017
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 15, 2017
@lunny
Copy link
Member

lunny commented Jan 15, 2017

But cannot delete attachments?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 15, 2017
@couling
Copy link
Contributor Author

couling commented Jan 15, 2017 •
edited
Loading

@lunny That's correct. I've left it out because there is no delete function for attachments against issues either. I think that should be a separate issue / PR to implement as this PR feels weighty enough and this PR doesn't make the situation any worse.

I've created #677 and will look to create a new PR for it when I get time.

@lunny
Copy link
Member

lunny commented Jan 15, 2017

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 15, 2017
@appleboy
Copy link
Member

appleboy commented Jan 15, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 15, 2017
@lunny lunny merged commit 64375d8 into go-gitea:master Jan 15, 2017
@couling couling deleted the attach-to-release branch January 15, 2017 15:56
This was referenced Jan 19, 2017
Closed
Closed
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lunny lunny lunny left review comments

+1 more reviewer

@appleboy appleboy appleboy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Milestone

1.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@couling @lunny @appleboy @tboerger