Fully support project and group avatars#1506
Conversation
2bc06bb to
7916da7
Compare
| } | ||
| type alias GroupAvatar | ||
| return json.Marshal((*alias)(a)) | ||
| } |
There was a problem hiding this comment.
Can you explain why this is needed?
There was a problem hiding this comment.
If we wouldn't have this the field wouldn't be sent. In the current implementation it will send an empty string which effectively means to remove the avatar, e.g.:
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/22" \
--data "avatar="
| type UpdateGroupOptions struct { | ||
| Name *string `url:"name,omitempty" json:"name,omitempty"` | ||
| Path *string `url:"path,omitempty" json:"path,omitempty"` | ||
| Avatar *GroupAvatar `url:"-" json:"avatar,omitempty"` |
There was a problem hiding this comment.
Shouldn't this be:
| Avatar *GroupAvatar `url:"-" json:"avatar,omitempty"` | |
| Avatar *GroupAvatar `url:"-" json:"-"` |
There was a problem hiding this comment.
hmmm, good question. I think in case of UploadRequest() the options are any ways never JSON encoded, but just form fields ... So, probably it doesn't matter. I have to double check.
|
Do you still need or want this? If so could you maybe cleanup the PR and reply to my questions? Then we can try to get it sorted... |
This change set adds support for project and group avatars in the same fashion this is already implemented for topics.
7916da7 to
02292ff
Compare
@svanharmelen I finally found some time to address this - sorry for the late response. Actually for both your question: I've pretty much just copy & pasted the approach from the topics endpoint. You've introduced the code here if I'm not mistaken. |
svanharmelen
left a comment
There was a problem hiding this comment.
Thanks @timofurrer... I'll take it 👍🏻
This change set adds support for project, group and user avatars in the
same fashion this is already implemented for topics.
Currently it's not possible to remove avatars from those API - I'm
exploring possible solutions
here.
Nonetheless, I think this PR can already be merged.