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/12/20 20:13:56 UTC
[GitHub] [airflow] ericpp opened a new pull request #20433: Moved `wait_for_state` inside of `TableauHook` context to ensure an a…
ericpp opened a new pull request #20433:
URL: https://github.com/apache/airflow/pull/20433
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
This PR should resolve issue #20432 which is caused by the `wait_for_state` call being executed outside of the `TableauHook` context after it has logged out of the Tableau server. This code moves the call into the context so that it is executed while still logged into the Tableau server.
--
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] ericpp commented on a change in pull request #20433: Moved `wait_for_state` inside context to ensure an active Tableau connection
Posted by GitBox <gi...@apache.org>.
ericpp commented on a change in pull request #20433:
URL: https://github.com/apache/airflow/pull/20433#discussion_r773597331
##########
File path: airflow/providers/tableau/operators/tableau.py
##########
@@ -107,6 +107,7 @@ def execute(self, context: dict) -> str:
error_message = f'Method not found! Available methods for {self.resource}: {available_methods}'
raise AirflowException(error_message)
+ job_id = ""
Review comment:
I added it in case the `TableauHook` context fails and it's left undefined. But I suppose if that were to happen, it would throw an exception and wouldn't reach `return job_id` anyway. I can remove it.
--
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] uranusjr commented on a change in pull request #20433: Moved `wait_for_state` inside context to ensure an active Tableau connection
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20433:
URL: https://github.com/apache/airflow/pull/20433#discussion_r773570443
##########
File path: airflow/providers/tableau/operators/tableau.py
##########
@@ -107,6 +107,7 @@ def execute(self, context: dict) -> str:
error_message = f'Method not found! Available methods for {self.resource}: {available_methods}'
raise AirflowException(error_message)
+ job_id = ""
Review comment:
This is unnecessary.
--
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 #20433: Moved `wait_for_state` inside context to ensure an active Tableau connection
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20433:
URL: https://github.com/apache/airflow/pull/20433#issuecomment-998570357
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] uranusjr merged pull request #20433: Ensure Tableau connection is active to access wait_for_state
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20433:
URL: https://github.com/apache/airflow/pull/20433
--
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