potiuk commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r459280204
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -235,3 +238,4 @@ def execute(self, context):
# If we can we return the url of the job
# for later use (like retrieving an artifact)
return build_info['url']
+ return None
Review comment:
It's a good practice to return a value consistently. If you return
something in one place and then do not return anything at the end, it's a very
likely case that you forgot the return.
It is pure manifestation of one of the [Python's Zen
principles](https://www.python.org/dev/peps/pep-0020/) - "Explicit is better
than implicit"
##########
File path: airflow/providers/jenkins/example_dags/example_jenkins_job_trigger.py
##########
@@ -44,7 +44,7 @@
job_trigger = JenkinsJobTriggerOperator(
task_id="trigger_job",
job_name="generate-merlin-config",
- parameters={"first_parameter": "a_value", "second_parameter": "18"},
+ parameters='{"first_parameter": "a_value", "second_parameter": "18"}',
Review comment:
I think it's because of the wrong type specified in the docstring (and
yes it should not be changed here)
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -94,11 +99,12 @@ class JenkinsJobTriggerOperator(BaseOperator):
@apply_defaults
def __init__(self,
- jenkins_connection_id,
- job_name,
- parameters="",
- sleep_time=10,
- max_try_before_job_appears=10,
+ jenkins_connection_id: str,
+ job_name: str,
+ parameters: str = "",
Review comment:
Actually, when you look at the "build_job_url" method code and
docstring, it should be Optional[Union[str, Dict, List]]
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -56,7 +60,7 @@ def jenkins_request_with_headers(jenkins_server, req):
# Jenkins's funky authentication means its nigh impossible to
distinguish errors.
if e.code in [401, 403, 500]:
raise JenkinsException(
- 'Error in request. Possibly authentication failed [%s]: %s' %
(e.code, e.msg)
+ 'Error in request. Possibly authentication failed [%s]: %s' %
(e.code, e.reason)
Review comment:
Nice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
