Re: [PR] add user_name to http git [airflow]
codenamelxl commented on code in PR #51256: URL: https://github.com/apache/airflow/pull/51256#discussion_r2310688280 ## providers/git/src/airflow/providers/git/hooks/git.py: ## @@ -68,6 +68,7 @@ def __init__( super().__init__() connection = self.get_connection(git_conn_id) self.repo_url = repo_url or connection.host +self.user_name = connection.login or "user" Review Comment: This is for the later logic which always enforces a "user_name". -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] add user_name to http git [airflow]
codenamelxl commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2310685898
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
The original one does not work for gitlab.
It requires a user_name even for auth_token case.
For github, either no user_name or bogus user_name works.
So I was thinking specifying it like this will achieve the most
compatibility. Please refer to:
https://github.com/apache/airflow/pull/51256#discussion_r213553640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
dstandish commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2310329149
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
what if user is not specified, and you don't want it to be specified?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
dstandish commented on code in PR #51256: URL: https://github.com/apache/airflow/pull/51256#discussion_r2310327207 ## providers/git/src/airflow/providers/git/hooks/git.py: ## @@ -68,6 +68,7 @@ def __init__( super().__init__() connection = self.get_connection(git_conn_id) self.repo_url = repo_url or connection.host +self.user_name = connection.login or "user" Review Comment: @codenamelxl it definitely seems wrong to have a default of "user" for username why does that make sense? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] add user_name to http git [airflow]
potiuk merged PR #51256: URL: https://github.com/apache/airflow/pull/51256 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] add user_name to http git [airflow]
potiuk commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2143215570
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
OK. Sounds good to me - but I guess others who were more involved in git
provider development should chime in here. @dstandish @ephraimbuddy
@jedcunningham - if you have any concerns?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
codenamelxl commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2143055623
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
Yes, I've tested `"whatever"` user work for github as well.
For Gitlab,
https://docs.gitlab.com/user/profile/personal_access_tokens/#clone-repository-using-personal-access-token
it's mentioned that the `user` can be any string and cannot be empty.
I don't have any Bitbucket to test. But its doc:
https://confluence.atlassian.com/bitbucketserver/http-access-tokens-939515499.html#:~:text=Or%20using%20Basic%20Auth%3A
seems to suggest that a `user` is required as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
potiuk commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2142696100
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
Will it work for github with "whatever" user as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
codenamelxl commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2137646020
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
The field is already optional when setting up the connection.
L71 set it to `"user"` should it be empty
For gitlab, the `user_name` doesn't have to be exactly the token name, a
bogus string like `user` is OK too. But without `user_name`, it will fail.
So I was thinking it might be more compatible going this way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
codenamelxl commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2137646020
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
The field is already optional when setting up the connection.
L71 set it to `"user"` should it be empty
For gitlab, the `user_name` doesn't have to be exactly the token name, a
bogus string like `user` is OK too. But without `user_name`, it will fail.
So I was thinking it's probably more compatible going this way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
potiuk commented on code in PR #51256:
URL: https://github.com/apache/airflow/pull/51256#discussion_r2135536404
##
providers/git/src/airflow/providers/git/hooks/git.py:
##
@@ -89,7 +90,7 @@ def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
if self.auth_token and self.repo_url.startswith("https://";):
-self.repo_url = self.repo_url.replace("https://";,
f"https://{self.auth_token}@";)
+self.repo_url = self.repo_url.replace("https://";,
f"https://{self.user_name}:{self.auth_token}@";)
Review Comment:
This should be optional. In GitHub it's ok for example to authenticate with
just token.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] add user_name to http git [airflow]
potiuk commented on PR #51256: URL: https://github.com/apache/airflow/pull/51256#issuecomment-2943728597 > Failed test seems not related to the change? Rebase to see . -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] add user_name to http git [airflow]
codenamelxl commented on PR #51256: URL: https://github.com/apache/airflow/pull/51256#issuecomment-2943141728 Failed test seems not related to the change? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
