Re: [PR] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
Lee-W merged PR #54207: URL: https://github.com/apache/airflow/pull/54207 -- 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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
Lee-W commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2342384205
##
providers/jenkins/tests/unit/jenkins/operators/test_jenkins_job_trigger.py:
##
@@ -116,6 +133,10 @@ def test_execute_job_polling_loop(self, parameters,
mocker):
operator.execute(None)
assert jenkins_mock.get_build_info.call_count == 2
+assert jenkins_mock.get_queue_item.call_count == 4
+
+assert mock_log.warning.call_count == 2
+assert mock_log.warning.call_args == mock.call("polling failed,
retrying", exc_info=True)
Review Comment:
```suggestion
assert mock_log.warning.mock_calls == [mock.call("polling
failed, retrying", exc_info=True)]
```
it probably looks like this
--
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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
Lee-W commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2342383329
##
providers/jenkins/tests/unit/jenkins/operators/test_jenkins_job_trigger.py:
##
@@ -116,6 +133,10 @@ def test_execute_job_polling_loop(self, parameters,
mocker):
operator.execute(None)
assert jenkins_mock.get_build_info.call_count == 2
+assert jenkins_mock.get_queue_item.call_count == 4
+
+assert mock_log.warning.call_count == 2
+assert mock_log.warning.call_args == mock.call("polling failed,
retrying", exc_info=True)
Review Comment:
it's just a random example.
--
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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
sjyangkevin commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2342386673
##
providers/jenkins/tests/unit/jenkins/operators/test_jenkins_job_trigger.py:
##
@@ -116,6 +133,10 @@ def test_execute_job_polling_loop(self, parameters,
mocker):
operator.execute(None)
assert jenkins_mock.get_build_info.call_count == 2
+assert jenkins_mock.get_queue_item.call_count == 4
+
+assert mock_log.warning.call_count == 2
+assert mock_log.warning.call_args == mock.call("polling failed,
retrying", exc_info=True)
Review Comment:
Cool! Got it, will push the changes soon. Thanks for the feedback.
--
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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
sjyangkevin commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2342368870
##
providers/jenkins/tests/unit/jenkins/operators/test_jenkins_job_trigger.py:
##
@@ -116,6 +133,10 @@ def test_execute_job_polling_loop(self, parameters,
mocker):
operator.execute(None)
assert jenkins_mock.get_build_info.call_count == 2
+assert jenkins_mock.get_queue_item.call_count == 4
+
+assert mock_log.warning.call_count == 2
+assert mock_log.warning.call_args == mock.call("polling failed,
retrying", exc_info=True)
Review Comment:
Thanks for the feedback. Will try to understand it more. In this case, since
no http request is sent. Should I adjust the assertions to a list of call which
outline the params for each call?
--
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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
Lee-W commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2342346883
##
providers/jenkins/tests/unit/jenkins/operators/test_jenkins_job_trigger.py:
##
@@ -116,6 +133,10 @@ def test_execute_job_polling_loop(self, parameters,
mocker):
operator.execute(None)
assert jenkins_mock.get_build_info.call_count == 2
+assert jenkins_mock.get_queue_item.call_count == 4
+
+assert mock_log.warning.call_count == 2
+assert mock_log.warning.call_args == mock.call("polling failed,
retrying", exc_info=True)
Review Comment:
```python
assert mock_http_run.mock_calls == [
call(
endpoint="api/v2/accounts/test_account_id/",
data=None,
extra_options=None,
)
]
```
nit: I think the airflow community decided to use this kind of style
--
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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
sjyangkevin commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2318875192
##
providers/jenkins/src/airflow/providers/jenkins/operators/jenkins_job_trigger.py:
##
@@ -143,47 +143,46 @@ def poll_job_in_queue(self, location: str,
jenkins_server: Jenkins) -> int:
queue without having a build number assigned. We have to wait until the
job exits the queue to know its build number.
-To do so, we add ``/api/json`` (or ``/api/xml``) to the location
-returned by the ``build_job`` call, and poll this file. When an
-``executable`` block appears in the response, the job execution would
-have started, and the field ``number`` would contains the build number.
+To do so, we use get_queue_item to get information about a queued item
+
https://python-jenkins.readthedocs.io/en/latest/api.html#jenkins.Jenkins.get_queue_item
:param location: Location to poll, returned in the header of the
build_job call
:param jenkins_server: The jenkins server to poll
:return: The build_number corresponding to the triggered job
"""
-location += "/api/json"
-# TODO Use get_queue_info instead
-# once it will be available in python-jenkins (v > 0.4.15)
self.log.info("Polling jenkins queue at the url %s", location)
+
+match = re.search(r"/queue/item/(\d+)/?", location)
+if not match:
+raise ValueError(f"Invalid queue location format: {location}")
+
+queue_id = int(match.group(1))
+
+self.log.info("Polling Jenkins queue item with ID %s", queue_id)
for attempt in range(self.max_try_before_job_appears):
if attempt:
time.sleep(self.sleep_time)
-
try:
-location_answer = jenkins_request_with_headers(
-jenkins_server, Request(method="POST", url=location)
-)
-# we don't want to fail the operator, this will continue to poll
-# until max_try_before_job_appears reached
+json_response: dict = jenkins_server.get_queue_item(queue_id)
Review Comment:
Thanks for capturing it. I think it could happen. Will add a fix and
introduce a new test case for this scenario.
--
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] Handle get_queue_info todo in JenkinsJobTriggerOperator [airflow]
Lee-W commented on code in PR #54207:
URL: https://github.com/apache/airflow/pull/54207#discussion_r2318261016
##
providers/jenkins/src/airflow/providers/jenkins/operators/jenkins_job_trigger.py:
##
@@ -143,47 +143,46 @@ def poll_job_in_queue(self, location: str,
jenkins_server: Jenkins) -> int:
queue without having a build number assigned. We have to wait until the
job exits the queue to know its build number.
-To do so, we add ``/api/json`` (or ``/api/xml``) to the location
-returned by the ``build_job`` call, and poll this file. When an
-``executable`` block appears in the response, the job execution would
-have started, and the field ``number`` would contains the build number.
+To do so, we use get_queue_item to get information about a queued item
+
https://python-jenkins.readthedocs.io/en/latest/api.html#jenkins.Jenkins.get_queue_item
:param location: Location to poll, returned in the header of the
build_job call
:param jenkins_server: The jenkins server to poll
:return: The build_number corresponding to the triggered job
"""
-location += "/api/json"
-# TODO Use get_queue_info instead
-# once it will be available in python-jenkins (v > 0.4.15)
self.log.info("Polling jenkins queue at the url %s", location)
+
+match = re.search(r"/queue/item/(\d+)/?", location)
+if not match:
+raise ValueError(f"Invalid queue location format: {location}")
+
+queue_id = int(match.group(1))
+
+self.log.info("Polling Jenkins queue item with ID %s", queue_id)
for attempt in range(self.max_try_before_job_appears):
if attempt:
time.sleep(self.sleep_time)
-
try:
-location_answer = jenkins_request_with_headers(
-jenkins_server, Request(method="POST", url=location)
-)
-# we don't want to fail the operator, this will continue to poll
-# until max_try_before_job_appears reached
+json_response: dict = jenkins_server.get_queue_item(queue_id)
Review Comment:
wouldn't `json_response` become unbound if `HTTPError` or `JenkinsException`
is raised?
##
providers/jenkins/src/airflow/providers/jenkins/operators/jenkins_job_trigger.py:
##
@@ -143,47 +143,46 @@ def poll_job_in_queue(self, location: str,
jenkins_server: Jenkins) -> int:
queue without having a build number assigned. We have to wait until the
job exits the queue to know its build number.
-To do so, we add ``/api/json`` (or ``/api/xml``) to the location
-returned by the ``build_job`` call, and poll this file. When an
-``executable`` block appears in the response, the job execution would
-have started, and the field ``number`` would contains the build number.
+To do so, we use get_queue_item to get information about a queued item
+
https://python-jenkins.readthedocs.io/en/latest/api.html#jenkins.Jenkins.get_queue_item
:param location: Location to poll, returned in the header of the
build_job call
:param jenkins_server: The jenkins server to poll
:return: The build_number corresponding to the triggered job
"""
-location += "/api/json"
-# TODO Use get_queue_info instead
-# once it will be available in python-jenkins (v > 0.4.15)
self.log.info("Polling jenkins queue at the url %s", location)
+
+match = re.search(r"/queue/item/(\d+)/?", location)
+if not match:
+raise ValueError(f"Invalid queue location format: {location}")
Review Comment:
```suggestion
if not (match := re.search(r"/queue/item/(\d+)/?", location)):
raise ValueError(f"Invalid queue location format: {location}")
```
##
providers/jenkins/src/airflow/providers/jenkins/operators/jenkins_job_trigger.py:
##
@@ -143,47 +143,46 @@ def poll_job_in_queue(self, location: str,
jenkins_server: Jenkins) -> int:
queue without having a build number assigned. We have to wait until the
job exits the queue to know its build number.
-To do so, we add ``/api/json`` (or ``/api/xml``) to the location
-returned by the ``build_job`` call, and poll this file. When an
-``executable`` block appears in the response, the job execution would
-have started, and the field ``number`` would contains the build number.
+To do so, we use get_queue_item to get information about a queued item
+
https://python-jenkins.readthedocs.io/en/latest/api.html#jenkins.Jenkins.get_queue_item
:param location: Location to poll, returned in the header of the
build_job call
:param jenkins_server: The jenkins server to poll
:return: The build_number corresponding to the triggered job
"
