Skip to content
This repository was archived by the owner on Jun 8, 2019. It is now read-only.

Added attachment releases data #83

Closed
wants to merge 1 commit into from

Conversation

stefan-lacatus
Copy link

Apparently #77 reverted the parts of the changes merged in #63.
Bringing the changes in again.

@lunny
Copy link
Member

lunny commented Dec 3, 2017

Added a assets array to the releases GET request. This is a required change for go-gitea/gitea#3075

Get releases GET /repos/:owner/:repo/releases[/:id] - now includes assets
List assets: GET /repos/:owner/:repo/releases/:id/assets
Get single asset: GET /repos/:owner/:repo/releases/assets/:id

@@ -28,6 +28,7 @@ type Release struct {
// swagger:strfmt date-time
PublishedAt time.Time `json:"published_at"`
Publisher *User `json:"author"`
Attachments []*Attachment `json:"assets"`
Copy link
Member

Choose a reason for hiding this comment

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

I removed this because the field was never populated in API endpoints. Releases that do in fact have attachments had an empty assets fields, which I thought was misleading.

I think we should only add it back if we have plans to populate it in the near future.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we do have plans: go-gitea/gitea#3075

@@ -48,6 +49,33 @@ func (c *Client) GetRelease(user, repo string, id int64) (*Release, error) {
return r, err
}

// ListReleaseAttachments gets all the assets of a release in a repository
func (c *Client) ListReleaseAttachments(user, repo string, id int64) ([]*Attachment, error) {
Copy link
Member

Choose a reason for hiding this comment

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

These functions were removed in #77, because the API endpoints they use do not exist. I think we should only bring them back if we have plans to implement the corresponding API endpoints in the near future.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we do have plans: go-gitea/gitea#3075

@lafriks
Copy link
Member

lafriks commented Dec 3, 2017

LGTM

func (c *Client) GetReleaseAttachment(user, repo string, releaseID int64, attachmentID int64) (*Attachment, error) {
attachment := new(Attachment)
err := c.getParsedResponse("GET",
fmt.Sprintf("/repos/%s/%s/releases/%d/assets/%d", user, repo, releaseID, attachmentID),
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

It is incorrect, but the modeling of github is different from what gitea has.
In github, as you can see, you don't specify the release id. Assets are associated with the releases of a repo (not to a specific one). In gitea, the mapping between an attachment and a repo is done through a specific release. Also, we are never referencing this url in other endpoints. In github, ListReleases also has links to asset metadata. We don't have something like that.
I think the best thing is to just remove this endpoint. Or, of course i welcome other suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could keep v1 not compitable with github but v3 comiptable.

Copy link
Member

Choose a reason for hiding this comment

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

@stefan-lacatus AFAIK, an asset is uniquely identified by its id. Although an attachment is associated with a particular issue or release, you do not need which issue or release in order to identify an asset by its id. So, as far as I can tell, supporting the Github-style route should not be a problem.

@jonasfranz
Copy link
Member

Should be close since it resolved already.

@lunny lunny closed this Apr 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants