-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
FLPATH-144 https://issues.redhat.com/browse/FLPATH-214 Signed-off-by: Yaron Dayagi <ydayagi@redhat.com>
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good. Please add tls support
|
||
@Override | ||
public @NonNull List<WorkParameter> getWorkFlowTaskParameters() { | ||
LinkedList<WorkParameter> params = new LinkedList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add TLS support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkliczewski TLS support is already included. The client uses TLS when the URL begins with https. The client uses the standard Linux CA mechanism.
protected HttpHeaders buildHttpHeaders(WorkContext workContext) throws RestClientException { | ||
HttpHeaders httpHeaders = new HttpHeaders(); | ||
|
||
httpHeaders.setContentType(MediaType.APPLICATION_JSON); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we provide basic/general/common config as a base class. user can use it as is or extend.
public @NonNull List<WorkParameter> getWorkFlowTaskParameters() { | ||
LinkedList<WorkParameter> params = new LinkedList<>(); | ||
params.add(WorkParameter.builder().key("url").type(WorkParameterType.TEXT).optional(false) | ||
.description("URL to send request to").build()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how it is in other places, but I'd extract the keys to constants or to a parameter class.
the need to repeat the exact key name and be aware of its type isn't great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand what u mean by 'repeat the key' and 'aware of its type'.
The key is defined with its type only once.
The user has to be aware of the type when the request is created.
Perhaps you can suggest a code edit so I will understand?
/lgtm |
@ydayagi would you like to add integration tests as part of this PR or in a new one and let this be merged? |
merge |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: masayag The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
FLPATH-144 https://issues.redhat.com/browse/FLPATH-214
The task can be used as a base class for other/enhanced REST tasks. The protected methods enable that. These methods enable custom building of request headers and custom response processing.