Fetch PRs by REF, instead of cloning another repository

juliogonzalez commented 7 years ago

Huge change to the way we handle Git operations:

Until today we cloned upstream, then we cloned the fork, always with all history. That was a problem for huge repositories, or for forks of private repositories.

This PR allows GitBot to fetch only the single commit that requires testing, and just directly from the upstream repository without needing access to the forked repository.

It also simplifies git_op.rb as much as possible, and almost prepares GitOp class and its functions to migrate easily to a ruby git library, or to be used outside GitBot (that will be a separate PR, we still need to take care of moving repo name and https outside, as a 'protocol' variable since we should also support git protocol)

Unit test test_gitop is also changed and can now use a REAL PR! :-)

And finally, as a bonus, GitBot will now:

Exit with error if any of the git commands fail.
Inform about the commit that it's being tested (local hash and remote repo, branch and hash), so the user can have this information at the logs and verify that the correct commit was tested.

juliogonzalez commented 7 years ago

Some local tests:

SSH forcing a failure:

$ ./gitbot.rb -r cd-training/gitbot -c lint-ruby -d 'Check ruby style guide compliance' -t '/tmp/test.sh' -u 'https://www.google.es' -g './gitrepo' -f '.rb'
==============================
TITLE_PR: Just a file for a fake PR, NR: 6
==============================
Initialized empty Git repository in /home/juliogonzalez/Documents/development/git/opensuse/gitbot/gitrepo/cd-training/.git/

remote: Counting objects: 30, done.
remote: Compressing objects: 100% (25/25), done.
remote: Total 30 (delta 0), reused 12 (delta 0), pack-reused 0
Unpacking objects: 100% (30/30), done.
From github.com:cd-training/gitbot
 * branch            refs/pull/6/head -> FETCH_HEAD

The following commit is to be tested:
- LOCAL HASH:  830e5677c6ff9370a43781dd76bffb8a31a51e39
- REPO ORIG:   juliogonzalez/gitbot
- BRANCH_ORIG: fake-pr
- HASH_ORIG:   830e5677c6ff9370a43781dd76bffb8a31a51e39

This is a fake test returning 1

SSH forcing a success:

$ ./gitbot.rb -r cd-training/gitbot -c lint-ruby -d 'Check ruby style guide compliance' -t '/tmp/test.sh' -u 'https://www.google.es' -g './gitrepo' -f '.rb'
==============================
TITLE_PR: Just a file for a fake PR, NR: 6
==============================
Initialized empty Git repository in /home/juliogonzalez/Documents/development/git/opensuse/gitbot/gitrepo/cd-training/.git/

remote: Counting objects: 30, done.
remote: Compressing objects: 100% (25/25), done.
remote: Total 30 (delta 0), reused 12 (delta 0), pack-reused 0
Unpacking objects: 100% (30/30), done.
From github.com:cd-training/gitbot
 * branch            refs/pull/6/head -> FETCH_HEAD

The following commit is to be tested:
- LOCAL HASH:  830e5677c6ff9370a43781dd76bffb8a31a51e39
- REPO ORIG:   juliogonzalez/gitbot
- BRANCH_ORIG: fake-pr
- HASH_ORIG:   830e5677c6ff9370a43781dd76bffb8a31a51e39

This is a fake test returning 0

HTTPS forcing a success:

$ ./gitbot.rb -r cd-training/gitbot -c lint-ruby -d 'Check ruby style guide compliance' -t '/tmp/test.sh' -u 'https://www.google.es' -g './gitrepo' -f '.rb' --https
==============================
TITLE_PR: Just a file for a fake PR, NR: 6
==============================
Initialized empty Git repository in /home/juliogonzalez/Documents/development/git/opensuse/gitbot/gitrepo/cd-training/.git/

remote: Counting objects: 30, done.
remote: Compressing objects: 100% (25/25), done.
remote: Total 30 (delta 0), reused 12 (delta 0), pack-reused 0
Unpacking objects: 100% (30/30), done.
From https://github.com/cd-training/gitbot
 * branch            refs/pull/6/head -> FETCH_HEAD

The following commit is to be tested:
- LOCAL HASH:  830e5677c6ff9370a43781dd76bffb8a31a51e39
- REPO ORIG:   juliogonzalez/gitbot
- BRANCH_ORIG: fake-pr
- HASH_ORIG:   830e5677c6ff9370a43781dd76bffb8a31a51e39

This is a fake test returning 0

HTTPS with no PRs requiring tests:

$ ./gitbot.rb -r cd-training/gitbot -c lint-ruby -d 'Check ruby style guide compliance' -t '/tmp/test.sh' -u 'https://www.google.es' -g './gitrepo' -f '.rb' --https
==============================
TITLE_PR: Just a file for a fake PR, NR: 6
==============================
success
==============================
TITLE_PR: Fake/rb, NR: 5
==============================
failure
==============================
TITLE_PR: Dockerfile and script to create and run an image to run rubocop, NR: 4
==============================
no files of type .rb found! skipping
==============================
TITLE_PR: Improve documentation, NR: 3
==============================
success

juliogonzalez commented 7 years ago

Strange, unit tests are working for me locally:

$ rake
rubocop 
Inspecting 23 files
.......................

23 files inspected, no offenses detected
rubocop Rakefile
Inspecting 1 file
.

1 file inspected, no offenses detected
/usr/bin/ruby.ruby2.4 helper.rb
Started with run options --seed 1300

  0/0: [==================================================================================================================================================================================================================================] 100% Time: 00:00:00, Time: 00:00:00

Finished in 0.00067s
0 tests, 0 assertions, 0 failures, 0 errors, 0 skips
/usr/bin/ruby.ruby2.4 t_gitbot_backend.rb
Started with run options --seed 64699

  /0: [=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=] 0% Time: 00:00:00,  ETA: ??:??:??

  2/2: [==================================================================================================================================================================================================================================] 100% Time: 00:00:00, Time: 00:00:00

Finished in 0.00496s
2 tests, 10 assertions, 0 failures, 0 errors, 0 skips
/usr/bin/ruby.ruby2.4 t_opt_parser.rb
Started with run options --seed 55988

Incorrect syntax: option --context not found---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=] 0% Time: 00:00:00,  ETA: ??:??:??

Use option -h for help
Incorrect syntax: option --test_file not found

Use option -h for help
  3/3: [==================================================================================================================================================================================================================================] 100% Time: 00:00:00, Time: 00:00:00

Finished in 0.00275s
3 tests, 11 assertions, 0 failures, 0 errors, 0 skips
/usr/bin/ruby.ruby2.4 t_git_operation.rb
Started with run options --seed 37357

gitty [=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=---=] 0% Time: 00:00:00,  ETA: ??:??:??
Initialized empty Git repository in /home/juliogonzalez/Documents/development/git/opensuse/gitbot/tests/unit_tests/gitty/openSUSE/.git/

remote: Counting objects: 19, done.
remote: Compressing objects: 100% (17/17), done.
remote: Total 19 (delta 0), reused 12 (delta 0), pack-reused 0
Unpacking objects: 100% (19/19), done.
From github.com:openSUSE/gitbot
 * branch            refs/pull/5/head -> FETCH_HEAD

  1/1: [==================================================================================================================================================================================================================================] 100% Time: 00:00:03, Time: 00:00:03

Finished in 3.02852s
1 tests, 0 assertions, 0 failures, 0 errors, 0 skips

juliogonzalez commented 7 years ago

Ok, of course it is failing!

I changed GitOp to exit on errors! :-)

The clone was failing on the previous PRs (https://travis-ci.org/openSUSE/gitbot/builds/280472754?utm_source=github_status&utm_medium=notification line 914) but GitBot was not exiting.

@MalloZup, we have two options here:

Fixing the test so it can detect and accept the error
Switch the test to use https (IMHO the best option because this way we can even see that the test is cloning a PR)

Let me know how do you want to proceed, and I will change the PR.

MalloZup commented 7 years ago

@juliogonzalez thx. :+1:

My only concern is following:

Assuming we have the Upstream-branch31 and a PR that want to merge there.

the actual behaviour is:

get latest changes of Upstream-branch 31 (where the PR goes in) ( a git branch always change)
from this Upsteam-branch on, we merge the PR-branch
then we run the tests with (Upstream + PR merged)

This is really different from only testing the latest commit from the PR-wantomerge ( we donno if the commit can work togheter with the latest upstream, when merged into it).

Afaik with your change, we will just trust the author of PR, that his branch is always up2date with upstream ( and trust a manual operation), because github let you do PRs that are not 1:1 with upstream.

This was my motivation at beginning, to do all the merge the PR to into Upstream branch, and test this togheter. ( i saw advantage on this, rather then test only the commit.)

As a side remark For cloning, the git clone is happening only 1 time in the normal gitbot, if the git_dir exist we don't clone everytime, which is what we do in production.

Open for discussion on this :smile:

juliogonzalez commented 7 years ago

Understood. As long as the developers KNOW about it, it is not a bad idea (as in the end you are not testing the code they have a the PR, which is what this PR does as you correctly guessed).

If we want to keep the current behaviour then we need two commits (we still don't need to clone):

The last from the parent branch
The last from the PR.

Then try to merge/rebase, exit with error if it doesn't succeed, and otherwise test the result.

But, anyway, since a lot of developers and teams want to test the actual PR code (and not a rebase), I will add an option (something like --only-pr-code) to enable it.

What about the test? Can I change it to use HTTPS so it can really test a PR fetch?

And about the side remark: you are right. I got confused because a previous PR with two clones mentioned (can't remember why) :-)

I will change PR for this after work, if I don't have time now.

juliogonzalez commented 7 years ago

And thanks for clarifying!

Discussions aside, it's clear we need to keep gitbot retrocompatible, and add more options if we want to change current behaviour.

Gitarro commented 7 years ago

Why did you need to use my username... Flooded my Email-account. :'D

juliogonzalez commented 7 years ago

@MalloZup, as I mentioned it's clear we need to change the command line to make the nickname configurable, and by default a non-valid git username.

MalloZup commented 7 years ago

@Gitarro sorry:rofl: . @juliogonzalez , i will just remove the @ so we are not quoting users :D

juliogonzalez commented 7 years ago

I would leave it optional. If the user specifies a username on CLI then it will use @, otherwise it will use gitarro

For Jenkins users it will be useful to have, for example @jenkinks-juliogonzalez

juliogonzalez commented 7 years ago

Since master rebase, this is not mergeable anymore.

Closing, fixing issues and adding retrocompatibility.

openSUSE / gitarro

Fetch PRs by REF, instead of cloning another repository #44