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 support for Gerrit review system #38

Merged
merged 1 commit into from Mar 6, 2015

Conversation

Spredzy
Copy link
Contributor

@Spredzy Spredzy commented Feb 10, 2015

This commits add supports gerrit system and will submit a review
everytime a change is made on the same branch.

The modulesync.yml file looks like

---                                                                                                                                                              
namespace: stackforge
git_base:  ssh://jdoe@review.openstack.org:29418/
branch: msync_foo
branch_prefix: HEAD:refs/publish/master/
pre_commit_script: openstack-commit-msg-hook.sh

When a user receive a -1, the user can simply re-run msync update --amend and a new patchset will be submited

@claytono
Copy link

Thanks for working on this! Other than a few long lines, it looks ok to me, but I'll defer to others with more experience on modulesync internals.

@Spredzy
Copy link
Contributor Author

Spredzy commented Feb 10, 2015

@cmurphy would you agree that I pass the options hash instead of passing each items separately? cc @Dvorak

}
else
opts = {:amend => true}
message = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines are extremely problematic and unnecessary. They cause the script to amend the latest commit rather than making a new commit, when the previous commit is likely to be a change already in master and not owned by the committer.

@cmurphy
Copy link
Contributor

cmurphy commented Feb 13, 2015

I'm a little frustrated that gerrit must be treated so differently...but looking at your changes and playing with git-review a bit, I think perhaps it doesn't have to be treated so differently.

I propose that instead of a 'git_provider' parameter, we instead just need a 'pre_commit_script' parameter. We can maybe have a 'contrib/' or 'scripts/' directory that contains something to check for the existence of and install the commit-msg hook. Right before repo.commit, we can run the script to install the hook. The user can supply a gerrit-style git_base parameter already, as well as supply 'refs/publish/master/branchname' as the branch name. We can add documentation to make this easier. There isn't any need, as far as I can see, to explicitly amend commits, as the pre-commit hook will do that automatically.

What do you think?

@Spredzy
Copy link
Contributor Author

Spredzy commented Feb 13, 2015

@cmurphy this PR has been reworked with the following changes :

  • Unification of parameter: Only the git_base parameter is now used, default being git@github.com: (no more git_user, git_provider_address)
  • Creation of the contrib/ folder: As suggested using a contrib/ folder for pre_commit_script. In the case of review.openstack.org, it will bring the commit-msg hook directly from review.openstack.org (ie. openstack-commit-msg-hook.sh)
  • New command available --amend and --amend --force : In a system of review (like gerrit) msync update --amend will be enough to create a new patchset, in a system like github, the user will have to explicitly specify --amend --force

I hope all the point abode have been addressed

@Spredzy Spredzy force-pushed the support_for_gerrit branch 2 times, most recently from c7b58df to df5c72f Compare February 13, 2015 11:41
@claytono
Copy link

Would it make sense to just remove the --amend option entirely and have the pre-commit script check if the change already includes a Change-ID and add it (amend) if it's not there? If someone is using Gerrit, then that's very likely the behavior they want, and they can modify the pre-commit script if not.

@Spredzy Spredzy force-pushed the support_for_gerrit branch 3 times, most recently from d1baf7d to d82602e Compare February 13, 2015 16:09
* git_base : The default URL to git clone from (Default: 'git@github.com:')
* namespace : Namespace of the projects to manage (Default: 'puppetlabs')
* branch : Branch to push to (Default: 'master')
* branch_prefix : Branch prefix if necessary (ie. HEAD:refs/publish/master/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of branch and branch_prefix could we have branch (which would be the local branch) and remote_branch (which would default to the value of branch)? That way the push looks like git push origin localbranchname:remotebranchname, so they could potentially have different names and we don't need to refer to HEAD:.

@cmurphy
Copy link
Contributor

cmurphy commented Feb 15, 2015

@Spredzy looking good! A couple more notes.

@Dvorak I'd prefer not to have --amend be implicit, as it has the potential to be destructive. Moreover, as much as possible, I'd like to not have to treat gerrit or any other git providers as special. Adding the --amend and --force options make them available to github/gitlab/gitolite users. In any case, if you were doing it by hand you would use --amend anyway, so I don't think it's a big deal to require it here.

opts_push = {}
if amend
opts_commit = {:amend => true}
message = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set this to nil? I've had a lot of cases where amending the commit also means changing the commit message.

@cmurphy cmurphy mentioned this pull request Feb 16, 2015
This commits add supports gerrit system and will submit a review
everytime a change is made on the same branch.
cmurphy added a commit that referenced this pull request Mar 6, 2015
Add support for Gerrit review system
@cmurphy cmurphy merged commit 40fa74a into voxpupuli:master Mar 6, 2015
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