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

Crash if no pipeline is run #104

Open
Jellby opened this issue May 19, 2018 · 2 comments
Open

Crash if no pipeline is run #104

Jellby opened this issue May 19, 2018 · 2 comments

Comments

@Jellby
Copy link

Jellby commented May 19, 2018

I was testing what happens if no pipeline is run for the MR. Here's what I got (using merge strategy, if it matters):

2018-05-19 15:19:53,666 INFO Commit id to merge '8a8a087a54ea54e464af4dcf2bd457933b00dbe7' (into: '946a97b54cbcd4a47f21a1bed828e87c715a401b')
2018-05-19 15:19:59,405 INFO Waiting for CI to pass
2018-05-19 15:20:00,419 ERROR Unexpected Exception
Traceback (most recent call last):
  File "/home/ubuntu/marge-bot/marge/job.py", line 59, in execute
    self.update_merge_request_and_accept(approvals)
  File "/home/ubuntu/marge-bot/marge/job.py", line 152, in update_merge_request_and_accept
    self.wait_for_ci_to_pass(source_project.id, actual_sha, merge_request.source_branch)
  File "/home/ubuntu/marge-bot/marge/job.py", line 224, in wait_for_ci_to_pass
    assert current_pipeline.sha == commit_sha
AssertionError

I guess this is because there's no pipeline at all (configured with only: master, and master is the target branch, not the source). A graceful handling and a post in the MR would have been much better.

@aschmolck
Copy link
Contributor

This looks like a bug. Note that --use-merge-strategy is experimental (and marked as such), and generally not super well behaved, we accepted an MR to add minimal support, but it's not very good yet and I just accepted another one (belatedly) that fixed a bad function call (#102, also see #72).

Meta question: Irrespective of its current state of implementation, are you sure you want to --use-merge-strategy in the first place? Note that you can still can have your source branches merged into master (rather than rebased) by just leaving the default MR strategy in gitlab active (which does that). You only need this options if it's important to you that updating the source branch to the latest commit in the target branch happens via merge rather than rebase (and I'm still not sure why one would want that).

@Jellby
Copy link
Author

Jellby commented May 21, 2018

I guess the crash happens when a pipeline was run for the branch, but not for its latest commit (assert current_pipeline.sha == commit_sha), which can happen if pipelines are run manually or if the CI configuration changes. And the problem is having an assert with no exception handling.

Regarding the merge strategy, I may change my mind, but I don't think rewriting (i.e., rebasing) commits that are already pushed is a good idea. I rebase commits in my local branches, but once it's pushed, it's better not to touch it. (By the way, doesn't rebasing break any signature the commits may have?) I also have to deal with a community of developers who are not git masters and have already been trained to use merge. What marge-bot does should not affect them much, but if there is a conflict and when they are going to fix it the remote repository has rebased the commits that they may have further developed on in their branches... I'm sure it will cause confusion.

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

No branches or pull requests

2 participants