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

GitLab CI external repo #1238

Merged
merged 6 commits into from
Jul 9, 2020
Merged

Conversation

philipqnguyen
Copy link
Contributor

@philipqnguyen philipqnguyen commented Jun 25, 2020

Resolves #1044

It is common for code repos to be hosted on Github while using
Gitlab only as a CI/CD service.

Info on using Gitlab only as CI/CD service for external repos:
https://docs.gitlab.com/ee/ci/ci_cd_for_external_repos/

Current limitations:

  • Unfortunately, Gitlab currently does not expose the external repo's
    url, so users will have to manually set the repo's URL as an ENV var
    DANGER_PROJECT_REPO_URL
    (for ex., DANGER_PROJECT_REPO_URL=https://github.com/procore/blueprinter).
  • Additionally, RequestSources::Github#validates_as_ci? is tricky now because
    the git remote -v returns origin as gitlab.com rather than github.com. The
    reasoning is that even though the actual repo is on Github.com, GitlabCI maintains a
    mirror on Gitlab.com and the CI checkout the code from the Gitlab.com mirror.


def slug_from(env)
if env["DANGER_PROJECT_REPO_URL"]
env["DANGER_PROJECT_REPO_URL"].split('/').last(2).join('/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gitlab doesn't seem to give us any env var that points to the external repo's URL. It only points to gitlab's mirror URL. Users will need to set this ENV var themselves

@@ -40,6 +40,10 @@ def get_pr_from_branch(repo_name, branch_name, owner)
end
end

def validates_as_ci?
true
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the base class, it is looking up the git remote origin to see if it contains github.com:

# @return [Boolean] whether scm.origins is a valid git repository or not
def validates_as_ci?
!!self.scm.origins.match(%r{#{Regexp.escape self.host}(:|/)(.+/.+?)(?:\.git)?$})
end

Unfortunately, RequestSources::Github#validates_as_ci? is tricky now because
the git remote -v returns origin gitlab.com rather than github.com. The
reasoning is that even though the actual repo is on Github.com, GitlabCI maintains a
mirror on Gitlab.com and the CI checkout the code from there.

I could change this to also accept an origin that matches gitlab.com if the ci_source is Gitlab, however I am not sure I understand why there is a need to validate this at all, especially if we are already performing the validates_as_api_source?.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably reasonable

@repo_slug = env["CI_MERGE_REQUEST_PROJECT_PATH"] || env["CI_PROJECT_PATH"]
@project_url = env["CI_MERGE_REQUEST_PROJECT_URL"] || env["CI_PROJECT_URL"]
@repo_slug = slug_from(env)
@project_url = env["DANGER_PROJECT_REPO_URL"] || env["CI_MERGE_REQUEST_PROJECT_URL"] || env["CI_PROJECT_URL"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On an unrelated note, there is a separate PR #1237 to remove this project_url as it does not seem to be used anymore.

@philipqnguyen
Copy link
Contributor Author

I applied your suggestions @yg
Please let me know if anything else requires changing.

@philipqnguyen
Copy link
Contributor Author

@orta What do you think of this and are there any changes you want to see?

Copy link
Member

@orta orta left a comment

Choose a reason for hiding this comment

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

The validates_as_ci is the bit that really throws me off here, I'm gonna accept because it does look like I'm never going to take the time to dive into that properly at this point.

@@ -40,6 +40,10 @@ def get_pr_from_branch(repo_name, branch_name, owner)
end
end

def validates_as_ci?
true
end
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably reasonable

@orta
Copy link
Member

orta commented Jul 9, 2020

If you rebase I'll merge 👍

philipqnguyen and others added 6 commits July 9, 2020 11:43
It is common for code repos to be hosted on Github while using
Gitlab only as a CI/CD service.

Info on using Gitlab only as CI/CD service for external repos:
https://docs.gitlab.com/ee/ci/ci_cd_for_external_repos/

Current limitations:
Unfortunately, Gitlab currently does not expose the external repo's
url, so we have to set it in an ENV var as DANGER_PROJECT_REPO_URL.
The previous specs assumes all the PRs (or merge requests) are
made on a Gitlab hosted code repository. We move those specs to
its own context.
The following tests are for methods that will have an alternate
value if the code repo is hosted on Github.
GitlabCI with a Github hosted repository will actually have its
git remote origin point to gitlab.com and not to github.com.
The reason is that GitlabCI maintains a mirror of the Github Repo
and that the git remote in the CI points to the mirror rather than
to the actual git repository on Github.

I'm not sure if I see a need for maintain this #validates_as_ci?
for RequestSources...
Brand typo

Co-authored-by: Yogi <me@yogi.codes>
@danger-public
Copy link

1 Error
🚫 Tests have failed, see below for more information.

Tests:

File Name
gitlab_spec.rb Danger::RequestSources::GitLab valid server response #update_pull_request! supports inline comments edits existing inline comment instead of creating a new one if file/line matches
gitlab_spec.rb Danger::RequestSources::GitLab valid server response #update_pull_request! supports inline comments deletes non-sticky comments if no violations are present
gitlab_spec.rb Danger::RequestSources::GitLab valid server response #update_pull_request! supports inline comments deletes inline comments if those are no longer relevant
gitlab_spec.rb Danger::RequestSources::GitLab valid server response #update_pull_request! supports inline comments skips inline comments for files that are not part of the diff

Generated by 🚫 Danger

@philipqnguyen
Copy link
Contributor Author

philipqnguyen commented Jul 9, 2020

@orta, I rebased the PR, but tests are failing because a new gitlab gem was released on July 6th, v4.16.0, and it contains a bug: NARKOZ/gitlab#574

I believe a solution would be to specify gitlab < 4.16.0

@orta
Copy link
Member

orta commented Jul 9, 2020

Nah, not need to lock. I don't want to be accidentally missing something in the future by locking the version. I'm OK with CI being red until it's figured out, there's enough greens to validate that you didn't break something.

@orta orta merged commit 8067dd5 into danger:master Jul 9, 2020
@NARKOZ
Copy link

NARKOZ commented Jul 12, 2020

@orta, I rebased the PR, but tests are failing because a new gitlab gem was released on July 6th, v4.16.0, and it contains a bug: NARKOZ/gitlab#574

I believe a solution would be to specify gitlab < 4.16.0

I've released version 4.16.1 with a fix.

@orta
Copy link
Member

orta commented Jul 13, 2020

<3

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.

Github + Gitlab CI, get_repo_source is getting gitlab (it's a mirror), but the code is on github
4 participants