Skip to content
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.

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

Add possibility to add assignees to pull request #196

Merged

Conversation

Berreek
Copy link
Contributor

@Berreek Berreek commented Oct 14, 2021

What does this change

Add parameter assignees (-a) to the run command that allows to specify assignees usernames that should be added to the created pull request.

What issue does it fix

Closes #194

Notes for the reviewer

I'm not sure about one change in this pull request which is worth to mention I guess. In current implementation for GitLab when you pass reviewers in the command they were converted to assignees in GitLab. I changed it here so reviewers are converted to reviewers and assignees to assignees. This is a breaking change I assume if someone relied on the previous behaviuour.

Checklist

  • Made sure the PR follows the CONTRIBUTING.md guidelines
  • Tests if something new is added

Add parameter assignees (-a) to the pull request command that allows to specify assignees usernames that should be added to the created pull request.
@lindell
Copy link
Owner

lindell commented Oct 14, 2021

I would say that the GitLab implementation before was a bug. Even through it's a breaking change, I think we are fine to fix it since Multi-gitter is still not 1.0.0 🙂

tests/table_test.go Outdated Show resolved Hide resolved
Copy link
Owner

@lindell lindell left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution ❤️

There seems to be a minor mistake in the test, and a file that isn't gofmt'ed, otherwise it look good to me 😄

cmd/cmd-run.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

usersWithMetadata = append(usersWithMetadata, bitbucketv1.UserWithMetadata{User: userWithLinks})
assignees, err := b.getUsersWithLinks(newPR.Assignees, client)
Copy link
Owner

Choose a reason for hiding this comment

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

@ryancurrah Do you have the ability to test out the new --assignees flag in bitbucket server? 🙂

@ryancurrah
Copy link
Contributor

What's the difference between assignees and reviewers?

@lindell
Copy link
Owner

lindell commented Oct 14, 2021

@ryancurrah From this response:
For "reviewer": someone you want to review the code. Not necessarily the person responsible for that area or responsible for merging the commit. Can be someone who worked on that chunk of code before, as GitHub auto-suggests.

For "assignee": up to the project's team/maintainer what it means and there's no strict definition. It can be the PR opener, or someone responsible for that area (who is going to accept the PR after the review is done or just close it). It's not up to GitHub to define what it is leaving it open for project maintainers what fits best for their project.

@Berreek Berreek marked this pull request as ready for review October 14, 2021 17:51
@Berreek
Copy link
Contributor Author

Berreek commented Oct 14, 2021

I applied suggestions from the code review and verified that it works for GitHub.

@ryancurrah
Copy link
Contributor

Hrm thats not a concept that exists in Bitbucket. Not sure how it will apply here. Maybe note that it wont work for certain SCM providers.

@lindell
Copy link
Owner

lindell commented Oct 15, 2021

Assignees do not exist, but "Participants" seems to be a concept. It seems that they should be similar, but I'm not really sure.

Copy link
Owner

@lindell lindell left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for the contribution 😄

The only thing I'm uncertain about is if we should do the assignee -> participant mapping for bitbucket server. I will let @ryancurrah decide that. Otherwise, I'm ready to merge it.

@ryancurrah
Copy link
Contributor

ryancurrah commented Oct 18, 2021

LGTM 👍 Thanks for the contribution 😄

The only thing I'm uncertain about is if we should do the assignee -> participant mapping for bitbucket server. I will let @ryancurrah decide that. Otherwise, I'm ready to merge it.

Looking at the API documentation a participant has three roles, AUTHOR, REVIEWER, or PARTICIPANT. - https://docs.atlassian.com/bitbucket-server/rest/7.17.0/bitbucket-rest.html#idp282

So participant is how a reviewer is added to a pull request.

And it looks like only REVIEWER role can be assigned.

Currently only the REVIEWER role may be assigned. - https://docs.atlassian.com/bitbucket-server/rest/7.17.0/bitbucket-rest.html#idp350

So it seems that in this case participant -> reviewer.

@lindell
Copy link
Owner

lindell commented Oct 18, 2021

Thanks @ryancurrah 👍 I guess in that case, we should remove the assignees for bitbucket server, since assigning participants are already reviewers?

@ryancurrah
Copy link
Contributor

Yes that seems to be the case.

@lindell
Copy link
Owner

lindell commented Oct 19, 2021

@Berreek If you remove the bitbucket server code, I'm ready to merge the PR 🙂

@Berreek
Copy link
Contributor Author

Berreek commented Oct 20, 2021

Done!

@lindell lindell merged commit 52cde84 into lindell:master Oct 20, 2021
lindell pushed a commit that referenced this pull request Oct 20, 2021
the previous mapping in GitLab was reviewers -> assignees. It has now been corrected.
@lindell
Copy link
Owner

lindell commented Oct 20, 2021

Released in v0.35.0 🎉

@Berreek Berreek deleted the ability-to-specify-assignees-to-pull-requests branch October 23, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to specify pull request assignees
3 participants