fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty

[marceloboeira]

In this particular example, mr.HeadPipeline.SHA panics on a nil
pointer dereference because HeadPipeline is empty.

This seems to be caused by the lack of permission to update the commit
status.

runtime.gopanic
        runtime/panic.go:1038
runtime.panicmem
        runtime/panic.go:221
runtime.sigpanic
        runtime/signal_unix.go:735
github.com/runatlantis/atlantis/server/events/vcs.(*GitlabClient).PullIsMergeable
        github.com/runatlantis/atlantis/server/events/vcs/gitlab_client.go:208
github.com/runatlantis/atlantis/server/events/vcs.(*ClientProxy).PullIsMergeable
        github.com/runatlantis/atlantis/server/events/vcs/proxy.go:72
github.com/runatlantis/atlantis/server/events/vcs.(*pullReqStatusFetcher).FetchPullStatus
        github.com/runatlantis/atlantis/server/events/vcs/pull_status_fetcher.go:28
github.com/runatlantis/atlantis/server/events.(*ApplyCommandRunner).Run
        github.com/runatlantis/atlantis/server/events/apply_command_runner.go:105
github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunCommentCommand
        github.com/runatlantis/atlantis/server/events/command_runner.go:252

The least invasive solution is to simply use the commit-hash from pull,
and guess that the pipeline was "skipped" unless the HeadPipeline is
there.

The outcome is:

When mr.HeadPipeline is present:

• use the commit hash and status from the HeadPipeline
  When mr.HeadPipeline is NOT present:
• use the commit hash from pull request struct
• assume the pipeline was "skipped"

In cases where GitLab is configured to require a pipeline to pass, this
results on a message saying the MR is not mergeable.

More info:

• runtime error: invalid memory address or nil pointer dereference #1852

[marceloboeira]

@sapslaj this is one way around the error, the least invasive I think...

Alternatively, we could simply assume that if mr.HeadPipeline is empty, something is wrong and return a more clear error...

I couldn't find unit tests for this specific Method, if there are any kinda of tests for the gitlab client that could cover this branch of the code I'm happy to create one for this odd scenario.

[jamengual]

by updating the go client library for gitlab do we get a proper status code? it sounds weird to me that we will "assume" is skipped just because we do not really know what is the real status.

I understand that this could be Gitlab API doing things that are not documented beforehand etc, maybe a support ticket to them might give us some light on what is really happening?

[marceloboeira]

as @sapslaj commented, we are already on the latest. We could open an issue there/ticket for support, but I would still think somehow it should be expected to withstand this failure, at least until the client doesn't handle it.

[jamengual]

I guess the problem is, who is going to create a cleanup pr after gitlab fixes the issue? most probably no one because the problem is being handled by the code instead of the client.

[marceloboeira]

@jamengual we could simply have a revert of this PR open and tie it to the gitlab-client, whenever they fix it this can be reversed it. Still, I would always defend my code against external problems that could lead to runtime errors.

[Youngboss657]

My Facebook clone is having some issues

[lkysow]

@marceloboeira is this the right fix? i.e. are we still checking the same things here? Or should we instead return an error? Since you said that:

Solution: create a new token for atlantis with the api permissions and try that. It should go around this issue!

[marceloboeira]

@lkysow

So, right now whenever this error happens there is a runtime exception with the stack trace. This PR is to prevent the runtime error, so that it gracefully shows an error message.

The only solution I found to make the atlantis apply work was to generate a new atlantis token.

[nishkrishnan]

Still, I would always defend my code against external problems that could lead to runtime errors.

In this case though there's no difference between what you're doing for this field and every other field in the event. That could get complex real quick. At some point you have to trust the API contract.

In general I think Atlantis has a bit too many "less invasive solutions" already bolted on. Would rather keep the code simple given that there is already a documented solution for this (creating a new token with api permissions).

[jamengual]

@marceloboeira closing after 30+ days

[nitrocode]

@marceloboeira could you continue to work on this if we reopen this?

[nitrocode]

Ah nevermind, just read through this and it seemed like it was decided against this in favor of the API giving us the correct response instead of working around it.