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

Use auth for GitLab private repos, and fix GitLab failover behavior #157

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

drzraf
Copy link
Contributor

@drzraf drzraf commented Oct 6, 2022

  • When retrieving a ZIP from a private repository, use authentication, if provided
  • Fix the GitLab failover behavior (used to bail-out on private GitLab repositories without trying to use a token if one was provided)

@drzraf drzraf requested a review from a team as a code owner October 6, 2022 15:47
@drzraf drzraf force-pushed the feature/gitlab-improvements branch from 64999f6 to 4e53a5d Compare October 6, 2022 16:45
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

@drzraf Should we support GITHUB_TOKEN too? Also, can you fix the PHP issues?

@drzraf
Copy link
Contributor Author

drzraf commented Oct 12, 2022

An even better way would be to rely upon composer infrastructure (since wp-cli already depends on it).
It would gives access to .config/composer/auth.json which is likely to contain all the tokens we needs.

Note that composer just provided a download-only flag (and public method) in composer/composer#11041 which would allow fetching packages (using authentication) and still install them the way we want.

NB: GITHUB_TOKEN is already supported in the code.

@drzraf
Copy link
Contributor Author

drzraf commented Oct 12, 2022

The PHPCS warning is unrelated to this patch and the suggested indentation change seems wrong. But committed a "fix" anyway.

@danielbachhuber
Copy link
Member

NB: GITHUB_TOKEN is already supported in the code.

@drzraf Cool. Just to clarify, GITHUB_TOKEN is supported by default, but it's necessary to add special support for GITLAB_TOKEN ?

@drzraf
Copy link
Contributor Author

drzraf commented Oct 13, 2022

I considered GITLAB_TOKEN (necessary for private GitLab repo) in the initial implementation. But it was buggy. GITLAB_TOKEN was either:

  • Not used (when it comes to the initial retrieval of composer.json)
  • The fallback logic (relying on GITLAB_TOKEN) for private packages, wasn't correct (and as such, private packages couldn't be retrieved)

@drzraf
Copy link
Contributor Author

drzraf commented Dec 21, 2022

ping ?

@danielbachhuber
Copy link
Member

@drzraf Is there a good way to test this?

@drzraf
Copy link
Contributor Author

drzraf commented Dec 22, 2022

GITLAB_TOKEN=xxx wp package install https://gitlab.com/my/project.git (my/project is a private repository)

  • Before: failed
  • After: succeed

@danielbachhuber
Copy link
Member

@drzraf Sorry for the delay!

Here's what I tried:

GITLAB_TOKEN=glpat-XXXXXX wp package install https://gitlab.com/danielbachhuber/wp-cli-test-package.git
Warning: Couldn't fetch default branch for package 'https://gitlab.com/danielbachhuber/wp-cli-test-package.git' (HTTP code 404). Presuming default branch is 'master'.
Installing package danielbachhuber/wp-cli-test-package (dev-master)
Updating /Users/danielbachhuber/wp-cli/packages/composer.json to require the package...
Registering https://gitlab.com/danielbachhuber/wp-cli-test-package.git as a VCS repository...
Using Composer to install the package...
---
Loading composer repositories with package information
Warning: Failed to execute git clone --mirror -- 'git@gitlab.com:danielbachhuber/wp-cli-test-package.git' '/Users/danielbachhuber/Library/Caches/composer/vcs/git-gitlab.com-danielbachhuber-wp-cli-test-package.git/'

Cloning into bare repository '/Users/danielbachhuber/Library/Caches/composer/vcs/git-gitlab.com-danielbachhuber-wp-cli-test-package.git'...
git@gitlab.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

---
Error: Package installation failed.
Reverted composer.json.

Is there something obvious I'm missing? My private repo is https://gitlab.com/danielbachhuber/wp-cli-test-package

drzraf added 2 commits August 30, 2023 10:26
…, if provided

- Fix the GitLab failover behavior (used to bail-out on private GitLab repositories without trying to use a token if one was provided)
@drzraf drzraf force-pushed the feature/gitlab-improvements branch from 59e7186 to e71b3e3 Compare August 30, 2023 13:26
@drzraf
Copy link
Contributor Author

drzraf commented Aug 30, 2023

Registering https://gitlab.com/danielbachhuber/wp-cli-test-package.git as a VCS repository shows that composer automatically switched to ssh to connect. Most probably, your ssh/ssh-agent/ssh-config is not configured or doesn't survive within a wp-cli run. (For me, it works). This is triggered by composer->setPreferSource(). The GITLAB_TOKEN environment (and HTTP headers) is only a way to access repository on HTTPS but doesn't do anything about ssh which must be configured on the machine (as for GitHub)

I just added a couple of commits fix two aspects:

  • Considering GitLab as a last fall-back for the case a package is given as an argument without precision.
  • Handle the GitLab subgroup (more than one / in the project name)

@danielbachhuber
Copy link
Member

@drzraf Cool, sounds good. Can you fix up the PHPCS issues and then we can land it?

@danielbachhuber danielbachhuber added this to the 2.3.3 milestone Sep 6, 2023
@danielbachhuber danielbachhuber changed the title Improvements regarding GitLab private packages handling Use auth for GitLab private repos, and fix GitLab failover behavior Sep 6, 2023
@danielbachhuber danielbachhuber self-requested a review September 6, 2023 21:10
@danielbachhuber danielbachhuber merged commit f295538 into wp-cli:main Sep 6, 2023
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants