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

Source Gitlab: Explicitly state api_url accepts URL without scheme (http or https) #21143

Closed
pgollangi opened this issue Jan 7, 2023 · 4 comments · Fixed by #21373
Closed
Labels
area/documentation Improvements or additions to documentation community team/documentation type/enhancement New feature or request

Comments

@pgollangi
Copy link
Contributor

pgollangi commented Jan 7, 2023

api_url of source gitlab only accepts URL without scheme (http or https) which is not clearly stated.

For example, if my gitlab instance is https://gitlab.myorganization.com, the api_url must be gitlab.myorganization.com, NOT https://gitlab.myorganization.com.

I figured out this hard way. Since I was trying to configure gitlab source with self-managed instance, I was under impression that gitlab connector not able to reach my self manages gitlab instance behind vpn, though I'm on VPN (I'm running locally)

I was getting below error when I use api_url with HTTPS:

Caught retryable error 'HTTPSConnectionPool(host='https', port=443): Max retries exceeded with url: 
/gitlab.xxxxxx.com/api/v4/groups?per_page=50
(Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0xffff8df3a880>: 
Failed to establish a new connection: [Errno -2] Name or service not known'))' after 4 tries. 
Waiting 40 seconds then retrying...

I believe update is required at both spec.json and docs page

@pgollangi pgollangi added area/documentation Improvements or additions to documentation type/enhancement New feature or request labels Jan 7, 2023
@pgollangi pgollangi changed the title gitlab docs: Explicitly state api_url accepts URL without scheme (http or https`) gitlab docs: Explicitly state api_url accepts URL without scheme (http or https) Jan 7, 2023
@juweins
Copy link
Contributor

juweins commented Jan 7, 2023

Hey @pgollangi ! Thanks for making this type of issue visible to us. I worked my way through the connector source code. I could confirm my first guess:

In the source code I found the issue related part: (from streams.py in airbyte-integrations/connectors/source-gitlab/source_gitlab/)

Index 40:

@property
def url_base(self) -> str:
        return f"https://{self.api_url}/api/v4/"

It appaears, that your issue is related to a hard coded url prefix. From here, we have two options:

  1. remove the hardcoded prefix and let user quickly paste their URL
  2. implement @pgollangi suggestion by adjusting the spec.json and documentation with a hint, e.g. "...without the scheme"

In my perception we should go for option one, since it does not result in a more complex Info/Instruction text which might be misleading or confusing for some users.
Furthermore, the default scheme of gitlab is via HTTPS, so even in the rare case of trying to paste a http-URL, there should be a proper check/adjustment in the source code anyway.

What do you think @pgollangi ? I would like to wait for a feedback from an airbyte employee or maintainer (@yevhenii-ldv @ykurochkin) before I proceed.

@pgollangi
Copy link
Contributor Author

pgollangi commented Jan 9, 2023

@juweins completely agree with you. It would be nice if option1 is implemented and accepts urls includes url scheme rather than confusing with bunch of instructions to use right url. option1 implementation should also take care of backward compatibly probably to continue to support existing URLs without schemes.

I would rather choose to accepts any URL with out without scheme. that way the code works for existing urls configured.

thoughts please.

@natalyjazzviolin natalyjazzviolin changed the title gitlab docs: Explicitly state api_url accepts URL without scheme (http or https) Source Gitlab: Explicitly state api_url accepts URL without scheme (http or https) Jan 9, 2023
@juweins
Copy link
Contributor

juweins commented Jan 9, 2023

Exactly, I agree with you and it should be backward compatible. There may be some investigations necessary in which way a possible solution (based on Opt 1) will affect the existing Gitlab connections.👍🏼

Do you want to propose a solution? Or can I give it a shot? 👍🏼

@pgollangi
Copy link
Contributor Author

@juweins created PR #21373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation community team/documentation type/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants