Skip to content

[FLINK-27185][connectors] Convert connector-jdbc module to assertj #19807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 8, 2022
Merged

[FLINK-27185][connectors] Convert connector-jdbc module to assertj #19807

merged 1 commit into from
Jun 8, 2022

Conversation

@alpreu
Copy link
Contributor

@alpreu alpreu commented May 24, 2022

Cherry-picked commits from #19660 to get the individual changes merged faster

@flinkbot
Copy link
Collaborator

flinkbot commented May 24, 2022 •
edited
Loading

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

snuyanzin
snuyanzin reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@snuyanzin snuyanzin left a comment

Choose a reason for hiding this comment

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

In general lgtm, I left some comments

@alpreu alpreu force-pushed the FLINK-27185-connector-jdbc branch from 6b10b67 to 4623827 Compare May 25, 2022 08:16
@alpreu
Copy link
Contributor Author

alpreu commented May 25, 2022

Thanks for taking the time to review @snuyanzin, I just pushed again with the requested changes

@alpreu alpreu force-pushed the FLINK-27185-connector-jdbc branch from 4623827 to 387a1f3 Compare May 25, 2022 09:49
@snuyanzin
Copy link
Contributor

snuyanzin commented May 25, 2022

LGTM as soon as the build completes successfully

@alpreu alpreu force-pushed the FLINK-27185-connector-jdbc branch from 387a1f3 to 097847a Compare May 30, 2022 10:44
@afedulov
Copy link
Contributor

afedulov commented Jun 1, 2022

LGTM

@XComp
Copy link
Contributor

XComp commented Jun 3, 2022

@alpreu Sorry for replying that late, but what about also migrating from junit4 to junit5 as part of that effort? Since we decided to go for assertj/junit5 in general? What's the motivation of moving to assertj only?

@alpreu
Copy link
Contributor Author

alpreu commented Jun 3, 2022

@alpreu Sorry for replying that late, but what about also migrating from junit4 to junit5 as part of that effort? Since we decided to go for assertj/junit5 in general? What's the motivation of moving to assertj only?

The migration to assertJ was mainly done using the script written by @slinkydeveloper (https://github.com/assertj/assertj-migrator). We did have to manually touch some files though because not all assertions worked out of the box and some had better alternatives. I believe there is another umbrella ticket tracking the Junit5 migration

@XComp
Copy link
Contributor

XComp commented Jun 3, 2022

I see, thanks for clarification. I guess, you're refering to FLINK-25325. I'm just wondering why there's not flink-connectors-jdbc mentioned in FLINK-25325. Any idea, @alpreu ?

XComp
XComp reviewed Jun 3, 2022
View reviewed changes
Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

Thanks, @alpreu . In the end, I decided to go over the diff once more as well because I was curious about the assertj migration. I identified a few nitty things which we might want to fix before merging...

@alpreu alpreu force-pushed the FLINK-27185-connector-jdbc branch from 097847a to d450d9f Compare June 7, 2022 08:47
@alpreu
Copy link
Contributor Author

alpreu commented Jun 7, 2022

I see, thanks for clarification. I guess, you're refering to FLINK-25325. I'm just wondering why there's not flink-connectors-jdbc mentioned in FLINK-25325. Any idea, @alpreu ?

There was no initial list of modules, people kept adding what they were working on. I agree that we should create a list for the sdk modules

XComp
XComp reviewed Jun 7, 2022
View reviewed changes
Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. It looks good now. Two question popped up, though, when doing my final pass over the change. Please see them below...

@alpreu @slinkydeveloper @snuyanzin
[FLINK-27185][connector] Convert connector-jdbc module to assertj
41d785a 
Co-authored-by: slinkydeveloper <francescoguard@gmail.com>
Co-authored-by: Sergey Nuyanzin <sergey.nuyanzin@aiven.io>
@alpreu alpreu force-pushed the FLINK-27185-connector-jdbc branch from d450d9f to 41d785a Compare June 7, 2022 13:33
XComp
XComp approved these changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@XComp
Copy link
Contributor

XComp commented Jun 8, 2022

The CI failure isn't related to the change but is caused by FLINK-27791. I'm going to go ahead and merge the PR.

@XComp XComp merged commit d0b3390 into apache:master Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snuyanzin snuyanzin snuyanzin left review comments

@XComp XComp XComp approved these changes

Assignees

No one assigned

Labels

component=<none>

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alpreu @flinkbot @snuyanzin @afedulov @XComp