remove workflow-service-sdk dependency from prebuilt-tasks #492
Conversation
| import static com.redhat.parodos.tasks.project.consts.ProjectAccessRequestConstant.ACCESS_REQUEST_APPROVAL_USERNAMES; | ||
| import static com.redhat.parodos.tasks.project.consts.ProjectAccessRequestConstant.ACCESS_REQUEST_ESCALATION_USERNAME; | ||
| import static com.redhat.parodos.tasks.project.consts.ProjectAccessRequestConstant.ACCESS_REQUEST_ID; |
There was a problem hiding this comment.
nit: static import are not allowed in production code according to code style doc
There was a problem hiding this comment.
Do we need to define this class here and in
I see @AllArgsConstructor and @NoArgsConstructor are missing here. Maybe we can define this class here and then use it in workflow-service (since it depends on prebuilt-tasks module).
WDYT?
There was a problem hiding this comment.
@AllArgsConstructor and @NoArgsConstructor are not really needed here as we built the object only once to make the api call in the task. On the other hand, the DTOs in workflow-service are meant for the APIs within which can also evolve. I'm in favour of keeping this class in prebuilt-tasks although it's a duplicate because we can easily remove the entire task if no longer needed with no impact in the APIs in workflow-service. Another point, it's consistency. If we move it in prebuilt-tasks, others in request and response from parodos/workflow-service/src/main/java/com/redhat/parodos/project/dto/* should be moved too but there are not used in any task.
| @Builder | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| public class AccessResponseDTO { |
There was a problem hiding this comment.
Same explanation above
| @Builder | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| public class AccessStatusResponseDTO { |
There was a problem hiding this comment.
Same explanation above
| workflow: | ||
| url: "${WORKFLOW_SERVER_URL:http://localhost:8080}" | ||
| auth: | ||
| basic: | ||
| user: test | ||
| password: test | ||
|
|
There was a problem hiding this comment.
to better understand: why is this change needed?
There was a problem hiding this comment.
It's been removed actually. It was needed to create the api client with the sdk approach.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story (FLPATH-xxxx):Fixes #
Change type
Impacted services
Checklist