Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

Add a workflow task that uses REST #341

Merged
merged 1 commit into from
May 21, 2023

Conversation

ydayagi
Copy link
Contributor

@ydayagi ydayagi commented May 16, 2023

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.

@openshift-ci openshift-ci bot requested review from eloycoto and lshannon May 16, 2023 22:24
FLPATH-144 https://issues.redhat.com/browse/FLPATH-214

Signed-off-by: Yaron Dayagi <ydayagi@redhat.com>
@lshannon
Copy link
Contributor

/lgtm

Copy link
Collaborator

@pkliczewski pkliczewski left a 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<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add TLS support

Copy link
Contributor Author

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be configurable?

Copy link
Contributor Author

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());
Copy link
Collaborator

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.

Copy link
Contributor Author

@ydayagi ydayagi May 18, 2023

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?

@pkliczewski
Copy link
Collaborator

/lgtm

@masayag
Copy link
Collaborator

masayag commented May 18, 2023

@ydayagi would you like to add integration tests as part of this PR or in a new one and let this be merged?

@ydayagi
Copy link
Contributor Author

ydayagi commented May 21, 2023

@ydayagi would you like to add integration tests as part of this PR or in a new one and let this be merged?

merge

@masayag
Copy link
Collaborator

masayag commented May 21, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented May 21, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit db8e234 into parodos-dev:main May 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants