Added UploadAvatar #1318
Added UploadAvatar #1318
Conversation
|
Any hope of having this merged in? |
|
Yes, but I'm kind a mega busy with my day job lately... Will get back on this next week of so. |
|
@mitar I thought it was maybe better to refactor it all in one go, so I took a stab at it and added a commit to your PR. I don't have good access to a GitLab instance, so it would be really cool if you could help me verify all 3 related methods ( If they all work as expected I'll merge this PR together with the other backwards incompatible PR. Thanks! |
Fixes xanzy#229.
svanharmelen
force-pushed
the
support-avatar
branch
from
45e217d to
979f1c9
Compare
|
Hmm... I just pushed another commit with another solution for the same logic. This one used a request option function. The advantage could be that this option can also be used when creating or editing a project, more inline with what @Bouwdie was trying to work towards earlier. I'm not sure which solution I like most, maybe the option function isn't needed and makes things more confusing for a user? What are your thoughts @mitar and @Bouwdie? |
|
It looks good. I like the new approach. |
mitar
force-pushed
the
support-avatar
branch
from
4a6c4b7 to
d12864e
Compare
|
I can confirm uploading avatar works. |
|
Why are you pusing the old version again? |
|
I still have the updated version locally, so I will push that and create a new PR from that one. Still not sure which approach the take. Using the |
|
Here is the branch with my changes and the two different solutions (see the individual commits) https://github.com/xanzy/go-gitlab/tree/support-avatar |
|
As for |
|
Oh, my mistake. I tried to rebase against the latest master. I think I made a mistake. Let me fix that. |
mitar
force-pushed
the
support-avatar
branch
from
d12864e to
4a6c4b7
Compare
Yes, I realized after that so I removed the comment. |
|
I restored the brnach. |
projects.go
Outdated
|
|
||
| // Set the buffer as the request body. | ||
| if err = req.SetBody(b); err != nil { | ||
| options = append(options, WithFile(avatar, "avatar.png", UploadAvatar)) |
There was a problem hiding this comment.
I do not think this works. You assume avatars are just .png files here. You should allow filename argument.
Disregard this. I was testing against my original version. Lol. |
Both look fine to me. |
|
I like the approach with the option. For the three methods that are now implemented it makes sense, but for the other functions that include a file as input it makes less sense. I do think that ideally we would like the *io.Reader to be part of the create or edit project options instead of a nullable parameter to the signature. If we can somehow provide a hint in the opt that it might have to deal with files and if it does switch over to the multipart body rendering. Because in the end that is what we are doing right, body in json we prefer and if there is a file upload involved switch over to the multipart. If we are ok with a little reflection, that checks for available reader instances in the opt struct and if they are not nil, we can which the body render impl. In the end a user checks out the api docs from gitlab and expects a property to be available in the opt stuct I think. Does this argumentation make sense to you to? 😃 |
|
Oh btw small note, we also need to update the go-querystring dependency to v1.1.0, pointers to slices were not rendered correctly, for example tags in the project options would not be rendered properly. 💯 |
Which endpoint requires both file upload and additional non-file parameters? |
For example create project https://docs.gitlab.com/ee/api/projects.html#create-project - where the avatar is just one of the request options. |
I think the issue with that is that Reader is not enough, you need also a filename, so that GitLab can figure out the MIME type (they seem to do that based on the file extension in the filename). |
|
Listening to both your feedback, I actually don't think the option func is very helpful as it probably makes it less clear how and when it can be used and it doesn't mimic a pattern that the GitLab API currently supports. So I think I prefer to use the "internal" Let me refactor and update the solution just a bit to see if we can also support the idea of switching based on a value in the |
|
@mitar agreed, so probably there will be a new struct type like uploadfile which encapsulates both the reader and the filename |
|
@svanharmelen I think if we have a tule for fileupload with a reference in the opt structs we can have with minimal reflection and a null check an indication whether we should switch to form based body rendering. Thx for both your time guys 😊 |
svanharmelen
force-pushed
the
support-avatar
branch
from
4a6c4b7 to
138e453
Compare
|
OK, updated the PR again and think it should be good like this. @Bouwdie I don't like to use reflection based on a specific type to detect if/when to switch the request format. Instead I prefer to check for that explicitly in the methods where it makes sense. Please let me know any final thoughts or concerns with this approach as I'm ready to merge this one. |
|
@svanharmelen perfectly fine not to use refection, it would only result in different behavior for a couple of methods anyway, so most of it would be useless overhead. |
|
Just a minor comment, otherwise it looks great! |
svanharmelen
force-pushed
the
support-avatar
branch
from
138e453 to
cff4b0b
Compare
|
@svanharmelen Thanks for the commits and the fix. I unfortunately am not used to the new review interface of github and didn't push my remarks. One this I would like to stress is to update the go-querystring dependency, refences to slices will otherwise not be encoded properly aka project.Tags[] for example. Thanks again 💯 |
|
I updated the dependency in my gitlab-config project and I can confirm that all recent changes work. |
|
Nice 👍🏻 |
Fixes #229.