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 2020/07/23 04:31:01 UTC
[GitHub] [airflow] coopergillan opened a new pull request #9947: [GH-9708] Add type annotation to providers/jenkins
coopergillan opened a new pull request #9947:
URL: https://github.com/apache/airflow/pull/9947
Set type hints for `JenkinsHook` and `jenkins_job_trigger`, ignoring a couple that seem to be slightly buggy.
<!--
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:
related: #9708
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667329610
Not sure if the Backport packages failure is due to one of my changes?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464466936
##########
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:
Yeah, I was thinking something like that. I will raise an issue.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464084377
##########
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:
Got it, that makes sense :+1:
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667598000
We have a merge conflict. Can you do a rebase?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463733626
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -195,9 +201,10 @@ def execute(self, context):
'Triggering the job %s on the jenkins : %s with the parameters : %s',
self.job_name, self.jenkins_connection_id, self.parameters)
jenkins_server = self.get_hook().get_jenkins_server()
- jenkins_response = self.build_job(jenkins_server)
- build_number = self.poll_job_in_queue(
- jenkins_response['headers']['Location'], jenkins_server)
+ jenkins_response = self.build_job(jenkins_server, self.parameters)
Review comment:
Here's where we preserve the same behavior for `self.build_job` by passing `self.parameters` as the "local" parameters in the function.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668620142
Done with a merge in.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667239312
@turbaszek - all right, had some good time to dig through this earlier this morning. Ended up with more changes than I expected, but am happy with where it ended up. Appreciate any other 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463480138
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -160,7 +163,7 @@ def poll_job_in_queue(self, location, jenkins_server):
location_answer = jenkins_request_with_headers(
jenkins_server, Request(method='POST', url=location))
if location_answer is not None:
- json_response = json.loads(location_answer['body'])
+ json_response = json.loads(location_answer['body']) # type: ignore
Review comment:
Type of `location_answer` is `Union[str, Mapping[str, str]]` and this is visible in L54:
```
return {'body': response_body.decode('utf-8'), 'headers': response_headers}
```
What I would try is:
```suggestion
body_str: str = location_answer['body']
json_response = json.loads(body_str)
```
The error comes from mypy assuming that the type under `body` key may be `Mapping` and that's is indeed and error as `json.loads` accepts only `str, bytes, bytearray`
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -160,7 +163,7 @@ def poll_job_in_queue(self, location, jenkins_server):
location_answer = jenkins_request_with_headers(
jenkins_server, Request(method='POST', url=location))
if location_answer is not None:
- json_response = json.loads(location_answer['body'])
+ json_response = json.loads(location_answer['body']) # type: ignore
Review comment:
Type of `location_answer` is `Union[str, Mapping[str, str]]` and this is visible in L54:
```
return {'body': response_body.decode('utf-8'), 'headers': response_headers}
```
What I would try is:
```suggestion
body_str: str = location_answer['body']
json_response = json.loads(body_str)
```
The error comes from mypy assuming that the type under `body` key may be `Mapping` and that is indeed an error as `json.loads` accepts only `str, bytes, bytearray`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463648465
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -56,7 +59,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.msg) # type: ignore
Review comment:
Awesome, that looks like it worked!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463732808
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -110,7 +114,9 @@ def __init__(self,
self.jenkins_connection_id = jenkins_connection_id
self.max_try_before_job_appears = max_try_before_job_appears
- def build_job(self, jenkins_server):
+ def build_job(
+ self, jenkins_server: Jenkins, parameters: Optional[str] = "",
Review comment:
Making parameters a local variable here allow us to make the type checking here easier. It also means we are no longer updating an attribute outside of `__init__`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668604391
@potiuk - this one should be ready to go. Last one from me with the GH- commit messages 😅
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668065589
> I picked Jenkins for my first typing PR since it looked small. Turns out there is a lot going on here!
Hah, type annotations can be tricky sometimes but I think it's always an interesting lesson 👌
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r459211038
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -56,7 +59,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.msg) # type: ignore
Review comment:
This one threw an error about `msg` not being an attribute of `HTTPError`. How would `mypy` know that?
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -197,7 +200,7 @@ def execute(self, context):
jenkins_server = self.get_hook().get_jenkins_server()
jenkins_response = self.build_job(jenkins_server)
build_number = self.poll_job_in_queue(
- jenkins_response['headers']['Location'], jenkins_server)
+ jenkins_response['headers']['Location'], jenkins_server) # type: ignore
Review comment:
I can't seem to get this one either. It keeps saying this, but I can't tell which one it wants me to change:
```
airflow/providers/jenkins/operators/jenkins_job_trigger.py:204: error: Invalid index type "str" fo
"Union[str, Mapping[str, str]]"; expected type "Union[int, slice]"
jenkins_response['headers']['Location'], jenkins_server)
^
```
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -160,7 +163,7 @@ def poll_job_in_queue(self, location, jenkins_server):
location_answer = jenkins_request_with_headers(
jenkins_server, Request(method='POST', url=location))
if location_answer is not None:
- json_response = json.loads(location_answer['body'])
+ json_response = json.loads(location_answer['body']) # type: ignore
Review comment:
I couldn't make sense of this one:
```
incompatible type "Union[str, Mapping[str, str]]"; expected "Union[str, bytes, bytearray]"
json_response = json.loads(location_answer['body'])
^
```
I can't tell which one should become `str, bytes, bytearray`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464014728
##########
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:
This looks like an unrelated change.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464127961
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -80,7 +86,7 @@ class JenkinsJobTriggerOperator(BaseOperator):
:param job_name: The name of the job to trigger
:type job_name: str
:param parameters: The parameters block to provide to jenkins. (templated)
- :type parameters: str
+ :type parameters: JSON blob, path to JSON file, Dict, or List
Review comment:
You mean essentially just put the exact type like you have here? That seems like wider practice, right? I can make that change.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan edited a comment on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan edited a comment on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668604391
@potiuk - this one should be ready to go. Last one from me with the GH-XXXX commit messages 😅
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464014698
##########
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:
```suggestion
parameters: Optional[Union[str, Dict]] = None,
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464275347
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -80,7 +86,7 @@ class JenkinsJobTriggerOperator(BaseOperator):
:param job_name: The name of the job to trigger
:type job_name: str
:param parameters: The parameters block to provide to jenkins. (templated)
- :type parameters: str
+ :type parameters: JSON blob, path to JSON file, Dict, or List
Review comment:
Yes, and you can add the information about json etc to `:param` part so users understand what the type mean
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464002397
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -123,20 +129,20 @@ def build_job(self, jenkins_server):
"""
# Warning if the parameter is too long, the URL can be longer than
# the maximum allowed size
- if self.parameters and isinstance(self.parameters, str):
+ if parameters and isinstance(parameters, str):
import ast
- self.parameters = ast.literal_eval(self.parameters)
+ parameters = ast.literal_eval(parameters)
- if not self.parameters:
- # We need a None to call the non parametrized jenkins api end point
- self.parameters = None
+ if not parameters:
+ # We need to a None to call the non parametrized jenkins api end point
+ parameters = None
request = Request(
method='POST',
- url=jenkins_server.build_job_url(self.job_name, self.parameters, None))
+ url=jenkins_server.build_job_url(self.job_name, parameters, None))
Review comment:
I worked with your feedback here and what I was seeing in the code and pushed something up just now.
I think this should be ready to go now. Thanks for all your help.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667601757
Yes, I can get to that soon. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463925080
##########
File path: airflow/providers/jenkins/hooks/jenkins.py
##########
@@ -29,7 +30,7 @@ class JenkinsHook(BaseHook):
Hook to manage connection to jenkins server
"""
- def __init__(self, conn_id='jenkins_default'):
+ def __init__(self, conn_id: Union[str, Text] = 'jenkins_default') -> None:
Review comment:
```suggestion
def __init__(self, conn_id: str = 'jenkins_default') -> None:
```
That's the usual type we use for connection id
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463482925
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -19,19 +19,22 @@
import json
import socket
import time
+from typing import Any, Mapping, Optional, Type, Union
from urllib.error import HTTPError, URLError
import jenkins
-from jenkins import JenkinsException
+from jenkins import Jenkins, JenkinsException
from requests import Request
from airflow.exceptions import AirflowException
from airflow.models import BaseOperator
from airflow.providers.jenkins.hooks.jenkins import JenkinsHook
from airflow.utils.decorators import apply_defaults
+JReqType = Mapping[str, Union[str, Mapping[str, str]]]
-def jenkins_request_with_headers(jenkins_server, req):
+
+def jenkins_request_with_headers(jenkins_server: Type[Jenkins], req: Request) -> JReqType:
Review comment:
If the result is always in type of `{'body': ..., 'headers': ...}` I would suggest introducing named tuple. This should solve a lot of problems and will add some useful stuff like accessing using attributes.
```python
class JenkingRequest(NamedTuple):
body: str
headers: Dict[str, str]
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463731243
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -160,7 +163,7 @@ def poll_job_in_queue(self, location, jenkins_server):
location_answer = jenkins_request_with_headers(
jenkins_server, Request(method='POST', url=location))
if location_answer is not None:
- json_response = json.loads(location_answer['body'])
+ json_response = json.loads(location_answer['body']) # type: ignore
Review comment:
I couldn't quite get this to work, but managed to pull something else together. See latest commit.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668617864
Can you rebase on top of master please :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464466008
##########
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:
> It seems like only the dictionary case is tested here. I'm thinking there should be test coverage for each of these cases, right?
It would be nice to use parametrize to test each of possible type
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464014893
##########
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:
This is mypa's expected behavior. If the function exits without calling `return`, Python implicitly returns None. Mypy requires us to be implicit in this case.
> Explicit is better than implicit.
https://www.python.org/dev/peps/pep-0020/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668044308
I picked Jenkins for my first typing PR since it looked small. Turns out there is a lot going on here!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463731564
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -19,19 +19,22 @@
import json
import socket
import time
+from typing import Any, Mapping, Optional, Type, Union
from urllib.error import HTTPError, URLError
import jenkins
-from jenkins import JenkinsException
+from jenkins import Jenkins, JenkinsException
from requests import Request
from airflow.exceptions import AirflowException
from airflow.models import BaseOperator
from airflow.providers.jenkins.hooks.jenkins import JenkinsHook
from airflow.utils.decorators import apply_defaults
+JReqType = Mapping[str, Union[str, Mapping[str, str]]]
-def jenkins_request_with_headers(jenkins_server, req):
+
+def jenkins_request_with_headers(jenkins_server: Type[Jenkins], req: Request) -> JReqType:
Review comment:
The headers have been a little difficult to pin down, so I went with a simpler approach.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463480138
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -160,7 +163,7 @@ def poll_job_in_queue(self, location, jenkins_server):
location_answer = jenkins_request_with_headers(
jenkins_server, Request(method='POST', url=location))
if location_answer is not None:
- json_response = json.loads(location_answer['body'])
+ json_response = json.loads(location_answer['body']) # type: ignore
Review comment:
Type of `location_answer` is `Union[str, Mapping[str, str]]` and this is visible in L54:
```
return {'body': response_body.decode('utf-8'), 'headers': response_headers}
```
What I would try is:
```suggestion
body_str: str = location_answer['body']
json_response = json.loads(body_str)
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463925330
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -123,20 +129,20 @@ def build_job(self, jenkins_server):
"""
# Warning if the parameter is too long, the URL can be longer than
# the maximum allowed size
- if self.parameters and isinstance(self.parameters, str):
+ if parameters and isinstance(parameters, str):
import ast
- self.parameters = ast.literal_eval(self.parameters)
+ parameters = ast.literal_eval(parameters)
- if not self.parameters:
- # We need a None to call the non parametrized jenkins api end point
- self.parameters = None
+ if not parameters:
+ # We need to a None to call the non parametrized jenkins api end point
Review comment:
```suggestion
# We need None to call the non parametrized jenkins api end point
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464084295
##########
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:
See comments after latest commit.
##########
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:
See latest update.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #9947:
URL: https://github.com/apache/airflow/pull/9947
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668066276
@turbaszek - any thoughts on the question about improving the unit tests? I asked the others a question here: https://github.com/apache/airflow/pull/9947#discussion_r464084255
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r459212064
##########
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 kinda made me add this after complaining about no return statement. It wouldn't even let me use just `return`. Is this a known issue/attribute of `mypy`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464113937
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -80,7 +86,7 @@ class JenkinsJobTriggerOperator(BaseOperator):
:param job_name: The name of the job to trigger
:type job_name: str
:param parameters: The parameters block to provide to jenkins. (templated)
- :type parameters: str
+ :type parameters: JSON blob, path to JSON file, Dict, or List
Review comment:
```
:param parameters: The parameters block to provide to jenkins. JSON blob, path to JSON file, Dict, or List. (templated)
:type parameters: Union[str, Dict, List]
```
I think the type should be explicit, WDYT?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667341964
Not sure why some of these got cancelled but happy to rebase if need be.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463925619
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -123,20 +129,20 @@ def build_job(self, jenkins_server):
"""
# Warning if the parameter is too long, the URL can be longer than
# the maximum allowed size
- if self.parameters and isinstance(self.parameters, str):
+ if parameters and isinstance(parameters, str):
import ast
- self.parameters = ast.literal_eval(self.parameters)
+ parameters = ast.literal_eval(parameters)
- if not self.parameters:
- # We need a None to call the non parametrized jenkins api end point
- self.parameters = None
+ if not parameters:
+ # We need to a None to call the non parametrized jenkins api end point
+ parameters = None
request = Request(
method='POST',
- url=jenkins_server.build_job_url(self.job_name, self.parameters, None))
+ url=jenkins_server.build_job_url(self.job_name, parameters, None))
Review comment:
I think at this point `parameters` have in fact `Optional[Any]` type due to `ast.literal_eval(parameters)` and this is incompatible with type `Optional[str]` of `build_job_url`.
Personally I would do:
```
params = ast.literal_eval(parameters) if parameters else None
url=jenkins_server.build_job_url(self.job_name, params, None)
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464468319
##########
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:
https://github.com/apache/airflow/issues/10132
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667474905
> Not sure if the Backport packages failure is due to one of my changes?
I restarted the jobs
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463478455
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -56,7 +59,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.msg) # type: ignore
Review comment:
Please try using `e.reason`:
```
@property
def reason(self):
return self.msg
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463731327
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -197,7 +200,7 @@ def execute(self, context):
jenkins_server = self.get_hook().get_jenkins_server()
jenkins_response = self.build_job(jenkins_server)
build_number = self.poll_job_in_queue(
- jenkins_response['headers']['Location'], jenkins_server)
+ jenkins_response['headers']['Location'], jenkins_server) # type: ignore
Review comment:
Handled this one with a simpler type check.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r463733766
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -195,9 +201,10 @@ def execute(self, context):
'Triggering the job %s on the jenkins : %s with the parameters : %s',
self.job_name, self.jenkins_connection_id, self.parameters)
jenkins_server = self.get_hook().get_jenkins_server()
- jenkins_response = self.build_job(jenkins_server)
- build_number = self.poll_job_in_queue(
- jenkins_response['headers']['Location'], jenkins_server)
+ jenkins_response = self.build_job(jenkins_server, self.parameters)
+ if jenkins_response:
Review comment:
This is to allow for the `return None` in these responses.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668687364
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-668067542
Just need to know if @potiuk is good with this one now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464437486
##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -80,7 +86,7 @@ class JenkinsJobTriggerOperator(BaseOperator):
:param job_name: The name of the job to trigger
:type job_name: str
:param parameters: The parameters block to provide to jenkins. (templated)
- :type parameters: str
+ :type parameters: JSON blob, path to JSON file, Dict, or List
Review comment:
@turbaszek - gave this a shot in the latest commit. Let me know what 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#issuecomment-667605216
@mik-laj just rebased, now hoping for a clean run!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] coopergillan commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r464084255
##########
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 changed this since I was seeing that `parameters` was a templated string. Now we have determined that it can be a JSON blob string, a path to a JSON file, a dictionary, or a list.
It seems like only the dictionary case is tested here. I'm thinking there should be test coverage for each of these cases, right?
If so, I can open a GH Issue. What do you think @mik-laj @potiuk?
----------------------------------------------------------------
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:
users@infra.apache.org