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

introduces writeSignoffCommit and requireScore features #1205

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

fuero
Copy link

@fuero fuero commented Feb 28, 2017

This pull request introduces 2 features to Gitblit:

writeSignoffCommit:
When using the merge button on a ticket, all reviewers are added in a 'Signed-off-by: reviewer reviewer@example.com' commit message line. Can be enabled/disabled in the same manner
as requireApproval.
Defaults to being disabled.

requireScore:
Allows specifying a particular score a ticket should have (i.e. a number of people who have to sign off on it) before the merge button appears in the web ui. Configurable per repo, requires the requireApproval setting.
Defaults to being disabled.

I hope the approach is clean enough and that other people want this as well.

This would save us from setting up Gerrit :-)

messageBuilder.append("\n\n");
for (Change change : ticket.getReviews(patchset)) {
UserModel ruser = gitblit.getUserModel(change.author);
messageBuilder.append(MessageFormat.format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it would add a sign-off for someone who gave a negative review but the total score exceeded the minimum requirement. Am I reading that wrong?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, you're correct. Have a look at the follow-up commit, this should fix it.

@gitblit
Copy link
Collaborator

gitblit commented Mar 1, 2017

@fuero I like this proposal. Do you consider it feature complete?

@gitblit
Copy link
Collaborator

gitblit commented Mar 1, 2017

I wonder if Reviewed-by would be more appropriate, see conventions.

@flaix
Copy link
Member

flaix commented Mar 1, 2017 via email

@fuero
Copy link
Author

fuero commented Mar 1, 2017

As I understand it, the definition of sign-off is somewhat fluent.

Another setting containing the header name to be added could be introduced.

The actual meaning of the sign-off line must be defined by the people relying on it (the repo owners).

As for it being feature complete: I've introduced this to meet our internal needs, which are evolving as well.

@flaix
Copy link
Member

flaix commented Mar 1, 2017

Aren't both sources agreeing, that Signed-off-by means that the signer has authorship of the code, while reviewed-by is for people reviewing?

Anyhow, as for an additional setting, I'd personally rather vote for using the existing setting, and if it has no value, nothing is added. I'm a friend of controlling setting proliferation.

Maybe the documentation should mention that this is only valid if a merge commit is created, or is that obvious enough for everyone?

@fuero
Copy link
Author

fuero commented Mar 6, 2017

@fzs I've adapted my code to reflect your idea. Is there a way to add the conventions link to message displayed in the GUI? It didn't seem to like HTML tags there.

@flaix
Copy link
Member

flaix commented Mar 7, 2017

That should work fine. But it is better to keep the link in code so that it can be changed more easily and allows for easier translation. See commit 8130906.

@fuero
Copy link
Author

fuero commented Apr 6, 2017

Hope this is ok, it does stick out from the other options now though.

@fuero
Copy link
Author

fuero commented Apr 10, 2017

rebased to match current origin master

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.

None yet

3 participants