You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/09 19:25:34 UTC
[GitHub] [airflow] samgans opened a new pull request #16916: Deprecate Tableau personal token authentication
samgans opened a new pull request #16916:
URL: https://github.com/apache/airflow/pull/16916
This PR closes #16669.
**Why:**
Authentication by personal token was not working properly since the Tableau provider's creation due to how the Tableau server client works (it invalidates previous personal token connection if one or more parallel are opened). Therefore, we decided to deprecate it.
*Changes:*
1. Remove `_auth_via_token` method from the `TableauHook` and make the documentation notes about the deprecation.
2. Fix `get_all` method of `TableauHook` to raise a verbose error when there is wrong object type was supplied.
3. Deprecate the usage of `TableauJobStatusSensor` in case of blocking `TableauRefreshWorkbookOperator` execution to remove the creation of duplicated connection (which is done by a sensor) and to improve the conformity of the class to SRP. Also fix some small documentation issues.
4. Deprecate testing of the personal token authentication and add change the logic of "blocked" task test.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] george79d commented on pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
george79d commented on pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#issuecomment-877418797
[geo](cyprus)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] samgans commented on pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
samgans commented on pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#issuecomment-877844428
Hi, @eladkal!
The thing here is that personal token authentication was not working correctly as intended in the scope of Airflow logic, that's why we decided to remove it. But you are right, such a breaking change is what we can avoid at this stage.
Therefore, I can propose to leave the changes I've done apart from removing the method for the authentication in the hook and tests for it. I will put it back and indicate the danger of using a personal token authentication approach in the documentation and the body of the method. 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
eladkal merged pull request #16916:
URL: https://github.com/apache/airflow/pull/16916
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#issuecomment-879609799
Some static checks are failing. I HEARTILY recommend to install pre-commit (as suggested in the error message). It will then even auto-correct everything for you @samgans
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a change in pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#discussion_r667366994
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -41,6 +40,16 @@ class TableauHook(BaseHook):
"""
Connects to the Tableau Server Instance and allows to communicate with it.
+ Can be used as a context manager: automatically authenticates the connection
+ when opened and signs out when closed.
+
+ WARNING: authenticaton by personal token was deprecated as Tableau automatically
+ invalidates opened personal token connection if one or more parallel
+ connections with the same token are opened. So, in the environments with
+ multiple parallel tasks this authentication method can lead to numerous bugs
+ and all the jobs will not run as they intended. Therefore personal token
+ auth option is no more available for the end user.
Review comment:
The place for this is in the docs:
https://github.com/apache/airflow/blob/main/docs/apache-airflow-providers-tableau/connections/tableau.rst
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] samgans edited a comment on pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
samgans edited a comment on pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#issuecomment-879826509
Hello, @potiuk . The thing is that static check asks me to change this:
```
warnings.warn(
"Authentication via personal access token is deprecated. "
"Please, use the password authentication to avoid inconsistencies.",
DeprecationWarning
)
```
To this:
```
warnings.warn(
"Authentication via personal access token is deprecated. "
"Please, use the password authentication to avoid inconsistencies.",
DeprecationWarning,
)
```
Which is not the thing we should have, I guess. What do you think?
**UPD**: I've rechecked the PEP8 and see that the trailing comma, in this case, is not redundant if we expect this to be extended. I think I will put it here. Thanks.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#issuecomment-880429702
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] samgans commented on pull request #16916: Deprecate Tableau personal token authentication
Posted by GitBox <gi...@apache.org>.
samgans commented on pull request #16916:
URL: https://github.com/apache/airflow/pull/16916#issuecomment-879826509
Hello, @potiuk . The thing is that static check asks me to change this:
```
warnings.warn(
"Authentication via personal access token is deprecated. "
"Please, use the password authentication to avoid inconsistencies.",
DeprecationWarning
)
```
To this:
```
warnings.warn(
"Authentication via personal access token is deprecated. "
"Please, use the password authentication to avoid inconsistencies.",
DeprecationWarning,
)
```
Which is not the thing we should have, I guess. What do you think?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org