Skip to content
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

feat: log warning in case of finding spring.jpa properties configuration #6291

Merged
merged 3 commits into from
Feb 17, 2020
Merged

feat: log warning in case of finding spring.jpa properties configuration #6291

merged 3 commits into from
Feb 17, 2020

Conversation

aureamunoz
Copy link
Member

Related to #6192

I'm working on the test but maybe can have already a feedback @geoand @gsmet

gsmet
gsmet reviewed Dec 20, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks.

I added a couple of suggestions.

gsmet
gsmet previously requested changes Dec 21, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Apparently, I missed the Request changes checkbox the first time. Adding a review to be extra sure we don't merge that one by accident.

@aureamunoz aureamunoz force-pushed the spring-conf-6192 branch 2 times, most recently from 1f1557b to 5b15786 Compare December 26, 2019 16:44
@aureamunoz
Copy link
Member Author

Hi, I added an IT with maven invoker plugin as @geoand suggested. I struggled with a problem of groovy version and a mix of maven plugin but I think now it's ok, at least the test. I will maybe refactor this #6291 (comment) but not sure what @gsmet means. Once you approve I will squash the commits.

@geoand
Copy link
Contributor

geoand commented Jan 13, 2020

@aureamunoz can you please rebase onto the latest master?

Thanks

@aureamunoz
Copy link
Member Author

done! Thank you @gsmet and @geoand :-)

@geoand geoand requested a review from gsmet January 13, 2020 15:36
@geoand
Copy link
Contributor

geoand commented Jan 31, 2020

@aureamunoz sorry this one slipped through the cracks!
Can you please rebase onto master just to be sure that nothing changed in the meanitime?

Thanks

@geoand
Copy link
Contributor

geoand commented Jan 31, 2020

@gsmet OK with this?

@geoand geoand added this to the 1.3.0 milestone Jan 31, 2020
@geoand geoand added the area/spring Issues relating to the Spring integration label Jan 31, 2020
@boring-cyborg boring-cyborg bot added the area/hibernate-orm Hibernate ORM label Feb 3, 2020
@aureamunoz
Copy link
Member Author

Done @geoand, rebase onto master :-)

@geoand geoand dismissed gsmet’s stale review February 3, 2020 11:12

Dismissing stale review since the blocking comment was addresse

@geoand
Copy link
Contributor

geoand commented Feb 3, 2020

Thanks @aureamunoz

gsmet
gsmet requested changes Feb 3, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added some comments and the assertions in the test look wrong.

@gsmet gsmet modified the milestones: 1.3.0.Alpha1, 1.3.0 Feb 4, 2020
@aureamunoz
Copy link
Member Author

I've just pushed the changes.Thanks for the review @gsmet, it worthed, there were a few things to fix 😅 Could you please re-check @gsmet ? or @geoand

@aureamunoz
Copy link
Member Author

What about this try? Let me know @Sanne @geoand @gsmet

geoand
geoand approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM

@Sanne Sanne requested a review from gsmet February 12, 2020 09:24
@gsmet gsmet modified the milestones: 1.3.0.Alpha2, 1.3.0 Feb 17, 2020
@aureamunoz
Copy link
Member Author

Hi, there is something to do from my side?

@geoand
Copy link
Contributor

geoand commented Feb 17, 2020

It's fine from my side. @gsmet can comment further if he thinks more changes need to be made

gsmet
gsmet approved these changes Feb 17, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's merge it.

My personal opinion is that the Map would have been more future proof when we will add more but I suppose we can redo it later when we add more.

@gsmet gsmet merged commit adb9743 into quarkusio:master Feb 17, 2020
@gsmet
Copy link
Member

gsmet commented Feb 17, 2020

Thanks!

@Sanne Sanne mentioned this pull request Mar 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sanne Sanne Sanne left review comments

@geoand geoand geoand approved these changes

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/hibernate-orm Hibernate ORM area/spring Issues relating to the Spring integration
Projects
None yet
Milestone
1.3.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@aureamunoz @geoand @gsmet @Sanne