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 JDBC #379

Merged
merged 1 commit into from
May 31, 2023

Conversation

ydayagi
Copy link
Contributor

@ydayagi ydayagi commented May 28, 2023

FLPATH-215 https://issues.redhat.com/browse/FLPATH-215

What this PR does / why we need it:
JDBC task for executing SQL queries/statements.
README file provides all details.
Which issue(s) this PR fixes:
FLPATH-215

Change type

  • New feature

Impacted services

  • Workflow Service

@openshift-ci openshift-ci bot requested review from nirarg and RichardW98 May 28, 2023 06:31
@ydayagi ydayagi force-pushed the flpath215 branch 2 times, most recently from 741f7dc to 7e53eeb Compare May 28, 2023 06:52
import java.util.List;
import java.util.Map;

interface JdbcService {
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls add doc comments to the interface and methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do u mean by doc comments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

javadoc, but based on the incremental approach, let's skip this in this PR.

@@ -30,6 +30,10 @@
</repositories>

<dependencies>
<dependency>
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls move next to other org.springframework.boot group dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 14 to 17
DriverManagerDataSource dataSource = new DriverManagerDataSource();
dataSource.setUrl(url);

JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is repetitive. the 4 lines above repeat themselves in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please suggest change

Copy link
Collaborator

Choose a reason for hiding this comment

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

extract lines 14-17 to a private function and reuse it in execute/update/query functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@ydayagi ydayagi force-pushed the flpath215 branch 4 times, most recently from cf6203b to 55bdc72 Compare May 30, 2023 05:44
@masayag
Copy link
Collaborator

masayag commented May 30, 2023

pls fix build errors, overall lgtm.

@ydayagi ydayagi force-pushed the flpath215 branch 3 times, most recently from a9a2fa1 to f181239 Compare May 30, 2023 12:39
@masayag
Copy link
Collaborator

masayag commented May 30, 2023

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented May 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pkliczewski

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

@ydayagi
Copy link
Contributor Author

ydayagi commented May 31, 2023

/retest

@ydayagi ydayagi force-pushed the flpath215 branch 2 times, most recently from 96a2170 to 09e02a8 Compare May 31, 2023 15:59
FLPATH-215 https://issues.redhat.com/browse/FLPATH-215

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

masayag commented May 31, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 31, 2023
@openshift-merge-robot openshift-merge-robot merged commit e7044b6 into rhdhorchestrator:main May 31, 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.

4 participants