-
Notifications
You must be signed in to change notification settings - Fork 18
SAC-31205 | Add on-premises GitLab instance support via api_url config field #51
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
Changes from all commits
fb8be69
a84bad9
0391413
14cd6a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,14 @@ | |
|
|
||
| setup( | ||
| name='tap-gitlab', | ||
| version='1.0.1', | ||
| version='1.1.0', | ||
| description='Singer.io tap for extracting data from the GitLab API', | ||
| author='Stitch', | ||
| url='https://singer.io', | ||
| classifiers=['Programming Language :: Python :: 3 :: Only'], | ||
| install_requires=[ | ||
| 'singer-python==6.1.1', | ||
| 'requests==2.33.0', | ||
| 'singer-python==6.8.0', | ||
| 'requests==2.34.2', | ||
| 'backoff==2.2.1' | ||
|
Comment on lines
6
to
16
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed |
||
| ], | ||
| entry_points=''' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| from typing import Any, Dict, Mapping, Optional, Tuple | ||
| import re | ||
|
|
||
| import backoff | ||
| import requests | ||
|
|
@@ -75,7 +76,15 @@ class Client: | |
| def __init__(self, config: Mapping[str, Any]) -> None: | ||
| self.config = config | ||
| self._session = session() | ||
| self.base_url = "https://gitlab.com/api/v4" | ||
|
|
||
| api_url = (config.get("api_url") or "").strip() or "https://gitlab.com" | ||
| api_url = api_url.rstrip("/") | ||
| if not api_url.startswith(("http://", "https://")): | ||
| api_url = f"https://{api_url}" | ||
| # Strip any existing /api/vN suffix so both 'https://gitlab.com' and | ||
| # 'https://gitlab.com/api/v4' are accepted without producing a double suffix. | ||
| api_url = re.sub(r"/api/v\d+$", "", api_url) | ||
| self.base_url = f"{api_url}/api/v4" | ||
|
|
||
| config_request_timeout = config.get("request_timeout") | ||
| self.request_timeout = float(config_request_timeout) if config_request_timeout else REQUEST_TIMEOUT | ||
|
|
@@ -93,12 +102,20 @@ def check_api_credentials(self) -> None: | |
| params = {'private_token': self.config.get("private_token")} | ||
| endpoint = f"{self.base_url}/user" | ||
|
|
||
| response = self._session.get( | ||
| endpoint, | ||
| params=params, | ||
| headers=headers, | ||
| timeout=self.request_timeout | ||
| ) | ||
| try: | ||
| response = self._session.get( | ||
| endpoint, | ||
| params=params, | ||
| headers=headers, | ||
| timeout=self.request_timeout | ||
| ) | ||
| except ConnectionError as e: | ||
| raise ConnectionError( | ||
| f"Unable to reach GitLab at '{self.base_url}'. " | ||
| "Please verify that 'api_url' in your config is correct and the host is reachable. " | ||
| f"({type(e).__name__})" | ||
| ) from e | ||
|
Comment on lines
+112
to
+117
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed
Comment on lines
+112
to
+117
|
||
|
|
||
| raise_for_error(response) | ||
|
|
||
| user_data = response.json() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,3 +100,88 @@ def test_generic_http_error(self, mock_request, mock_auth, mock_check_creds, moc | |
| with self.assertRaises(Error): | ||
| with Client(config) as client: | ||
| client.get(endpoint="dummy", params={}, headers={}) | ||
|
|
||
|
|
||
| class TestClientBaseUrl(unittest.TestCase): | ||
|
|
||
| def test_default_base_url_uses_gitlab_cloud(self): | ||
| """When api_url is absent the client targets gitlab.com.""" | ||
| config = {"private_token": "dummy_token"} | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.com/api/v4") | ||
|
|
||
| def test_empty_api_url_falls_back_to_gitlab_cloud(self): | ||
| """An empty or whitespace-only api_url must fall back to gitlab.com.""" | ||
| for value in ["", " ", None]: | ||
| with self.subTest(api_url=value): | ||
| config = {"private_token": "dummy_token", "api_url": value} | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.com/api/v4") | ||
|
|
||
| def test_custom_api_url_sets_base_url(self): | ||
| """When api_url is set the client targets the on-prem instance.""" | ||
| config = { | ||
| "private_token": "dummy_token", | ||
| "api_url": "https://gitlab.mycompany.com", | ||
| } | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.mycompany.com/api/v4") | ||
|
Comment on lines
+121
to
+128
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed |
||
|
|
||
| def test_trailing_slash_in_api_url_is_stripped(self): | ||
| """Trailing slashes in api_url must not produce a double-slash in base_url.""" | ||
| config = { | ||
| "private_token": "dummy_token", | ||
| "api_url": "https://gitlab.mycompany.com/", | ||
| } | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.mycompany.com/api/v4") | ||
|
|
||
| def test_api_url_without_scheme_defaults_to_https(self): | ||
| """An api_url with no scheme (e.g. 'gitlab.mycompany.com') must get https:// prepended.""" | ||
| config = { | ||
| "private_token": "dummy_token", | ||
| "api_url": "gitlab.mycompany.com", | ||
| } | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.mycompany.com/api/v4") | ||
|
|
||
| def test_api_url_with_port_and_no_scheme_gets_https(self): | ||
| """host:port without a scheme must not be mis-detected by urlparse and must get https://.""" | ||
| config = { | ||
| "private_token": "dummy_token", | ||
| "api_url": "gitlab.mycompany.com:8443", | ||
| } | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.mycompany.com:8443/api/v4") | ||
|
|
||
| def test_api_url_with_existing_api_v4_suffix_is_not_doubled(self): | ||
| """Providing a full API base URL must not produce /api/v4/api/v4.""" | ||
| config = { | ||
| "private_token": "dummy_token", | ||
| "api_url": "https://gitlab.mycompany.com/api/v4", | ||
| } | ||
| client = Client(config) | ||
| self.assertEqual(client.base_url, "https://gitlab.mycompany.com/api/v4") | ||
|
|
||
|
|
||
| class TestCheckApiCredentials(unittest.TestCase): | ||
|
|
||
| @patch("requests.Session.get", side_effect=ConnectionError("Failed to resolve 'test.gitlab.com'")) | ||
| def test_unreachable_host_raises_friendly_connection_error(self, mock_get): | ||
| """A DNS/network failure in check_api_credentials raises a descriptive ConnectionError | ||
| that does not leak the raw urllib3 message (which may contain the private_token URL).""" | ||
| config = { | ||
| "private_token": "dummy_token", | ||
| "api_url": "https://test.gitlab.com", | ||
| } | ||
| client = Client(config) | ||
| with self.assertRaises(ConnectionError) as ctx: | ||
| client.check_api_credentials() | ||
| msg = str(ctx.exception) | ||
| self.assertIn("Unable to reach GitLab", msg) | ||
| # base_url (no token) is shown, not the raw original error | ||
| self.assertIn("test.gitlab.com", msg) | ||
| self.assertIn("api_url", msg) | ||
| # Raw urllib3 message must not be included to avoid token leakage | ||
| self.assertNotIn("Failed to resolve", msg) | ||
| self.assertNotIn("private_token", msg) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed