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 2022/11/13 08:13:54 UTC
[GitHub] [airflow] eladkal commented on a diff in pull request #27633: Changing atlassian JIRA SDK to official atlassian-python-api SDK
eladkal commented on code in PR #27633:
URL: https://github.com/apache/airflow/pull/27633#discussion_r1020859178
##########
airflow/providers/atlassian/jira/operators/jira.py:
##########
@@ -76,16 +76,11 @@ def execute(self, context: Context) -> Any:
hook = JiraHook(jira_conn_id=self.jira_conn_id)
resource = hook.client
- # Current Jira-Python SDK (1.0.7) has issue with pickling the jira response.
- # ex: self.xcom_push(context, key='operator_response', value=jira_response)
- # This could potentially throw error if jira_result is not picklable
jira_result = getattr(resource, self.method_name)(**self.jira_method_args)
if self.result_processor:
return self.result_processor(context, jira_result)
return jira_result
- except JIRAError as jira_error:
- raise AirflowException(f"Failed to execute jiraOperator, error: {str(jira_error)}")
except Exception as e:
raise AirflowException(f"Jira operator error: {str(e)}")
Review Comment:
same comment/question
##########
airflow/providers/atlassian/jira/sensors/jira.py:
##########
@@ -96,38 +93,38 @@ def __init__(
if field_checker_func is None:
field_checker_func = self.issue_field_checker
- super().__init__(jira_conn_id=jira_conn_id, result_processor=field_checker_func, **kwargs)
+ super().__init__(
+ jira_conn_id=jira_conn_id, method_name="issue", result_processor=field_checker_func, **kwargs
+ )
def poke(self, context: Context) -> Any:
self.log.info("Jira Sensor checking for change in ticket: %s", self.ticket_id)
self.method_name = "issue"
- self.method_params = {"id": self.ticket_id, "fields": self.field}
+ self.method_params = {"key": self.ticket_id, "fields": self.field}
return JiraSensor.poke(self, context=context)
- def issue_field_checker(self, issue: Issue) -> bool | None:
+ def issue_field_checker(self, jira_result: dict) -> bool | None:
"""Check issue using different conditions to prepare to evaluate sensor."""
result = None
try:
- if issue is not None and self.field is not None and self.expected_value is not None:
+ if jira_result is not None and self.field is not None and self.expected_value is not None:
- field_val = getattr(issue.fields, self.field)
+ field_val = jira_result.get("fields", {}).get(self.field, None)
if field_val is not None:
if isinstance(field_val, list):
result = self.expected_value in field_val
elif isinstance(field_val, str):
result = self.expected_value.lower() == field_val.lower()
- elif isinstance(field_val, Resource) and getattr(field_val, "name"):
- result = self.expected_value.lower() == field_val.name.lower()
+ elif isinstance(field_val, dict) and field_val.get("name", None):
+ result = self.expected_value.lower() == field_val.get("name", "").lower()
else:
self.log.warning(
"Not implemented checker for issue field %s which "
"is neither string nor list nor Jira Resource",
self.field,
)
- except JIRAError as jira_error:
- self.log.error("Jira error while checking with expected value: %s", jira_error)
except Exception:
self.log.exception("Error while checking with expected value %s:", self.expected_value)
Review Comment:
same question
##########
airflow/providers/atlassian/jira/operators/jira.py:
##########
@@ -76,16 +76,11 @@ def execute(self, context: Context) -> Any:
hook = JiraHook(jira_conn_id=self.jira_conn_id)
resource = hook.client
- # Current Jira-Python SDK (1.0.7) has issue with pickling the jira response.
- # ex: self.xcom_push(context, key='operator_response', value=jira_response)
- # This could potentially throw error if jira_result is not picklable
jira_result = getattr(resource, self.method_name)(**self.jira_method_args)
if self.result_processor:
return self.result_processor(context, jira_result)
Review Comment:
If we remove the notes since no longer relevant shouldn't the code change as well?
to my understanding this code is a workaround to address the issue in comments
##########
airflow/providers/atlassian/jira/hooks/jira.py:
##########
@@ -43,48 +43,51 @@ def __init__(self, jira_conn_id: str = default_conn_name, proxies: Any | None =
super().__init__()
self.jira_conn_id = jira_conn_id
self.proxies = proxies
- self.client: JIRA | None = None
+ self.client: Jira | None = None
self.get_conn()
- def get_conn(self) -> JIRA:
+ def get_conn(self) -> Jira:
if not self.client:
self.log.debug("Creating Jira client for conn_id: %s", self.jira_conn_id)
- get_server_info = True
- validate = True
- extra_options = {}
+ verify = True
if not self.jira_conn_id:
raise AirflowException("Failed to create jira client. no jira_conn_id provided")
conn = self.get_connection(self.jira_conn_id)
if conn.extra is not None:
extra_options = conn.extra_dejson
# only required attributes are taken for now,
- # more can be added ex: async, logging, max_retries
+ # more can be added ex: timeout, cloud, session
# verify
if "verify" in extra_options and extra_options["verify"].lower() == "false":
- extra_options["verify"] = False
+ verify = False
# validate
- if "validate" in extra_options and extra_options["validate"].lower() == "false":
- validate = False
-
- if "get_server_info" in extra_options and extra_options["get_server_info"].lower() == "false":
- get_server_info = False
+ if "validate" in extra_options:
+ warnings.warn(
+ "Passing 'validate' in the connection is no longer supported.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+
+ if "get_server_info" in extra_options:
+ warnings.warn(
+ "Passing 'get_server_info' in the connection is no longer supported.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review Comment:
I think we don't really need this warnings?
we are changing SDKs.. parameters of the previous SDK can just cause confusion.
If users didn't change their usage of the hook nothing won't work anyway.
##########
airflow/providers/atlassian/jira/provider.yaml:
##########
@@ -22,11 +22,12 @@ description: |
`Atlassian Jira <https://www.atlassian.com/>`__
versions:
+ - 2.0.0
Review Comment:
no need for this here. the release manager decide the version upon release
##########
airflow/providers/atlassian/jira/hooks/jira.py:
##########
@@ -43,48 +43,51 @@ def __init__(self, jira_conn_id: str = default_conn_name, proxies: Any | None =
super().__init__()
self.jira_conn_id = jira_conn_id
self.proxies = proxies
- self.client: JIRA | None = None
+ self.client: Jira | None = None
self.get_conn()
- def get_conn(self) -> JIRA:
+ def get_conn(self) -> Jira:
if not self.client:
self.log.debug("Creating Jira client for conn_id: %s", self.jira_conn_id)
- get_server_info = True
- validate = True
- extra_options = {}
+ verify = True
if not self.jira_conn_id:
raise AirflowException("Failed to create jira client. no jira_conn_id provided")
conn = self.get_connection(self.jira_conn_id)
if conn.extra is not None:
extra_options = conn.extra_dejson
# only required attributes are taken for now,
- # more can be added ex: async, logging, max_retries
+ # more can be added ex: timeout, cloud, session
# verify
if "verify" in extra_options and extra_options["verify"].lower() == "false":
- extra_options["verify"] = False
+ verify = False
# validate
- if "validate" in extra_options and extra_options["validate"].lower() == "false":
- validate = False
-
- if "get_server_info" in extra_options and extra_options["get_server_info"].lower() == "false":
- get_server_info = False
+ if "validate" in extra_options:
+ warnings.warn(
+ "Passing 'validate' in the connection is no longer supported.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+
+ if "get_server_info" in extra_options:
+ warnings.warn(
+ "Passing 'get_server_info' in the connection is no longer supported.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
try:
- self.client = JIRA(
- conn.host,
- options=extra_options,
- basic_auth=(conn.login, conn.password),
- get_server_info=get_server_info,
- validate=validate,
+ self.client = Jira(
+ url=conn.host,
+ username=conn.login,
+ password=conn.password,
+ verify_ssl=verify,
proxies=self.proxies,
)
- except JIRAError as jira_error:
+ except Exception as jira_error:
raise AirflowException(f"Failed to create jira client, jira error: {str(jira_error)}")
Review Comment:
I don't think we need to catch the exceptions at all?
It will just raise whatever exception coming from the sdk
--
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