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.

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

Add more info about currentSchema property #1481

Merged
merged 1 commit into from Jun 28, 2019

Conversation

@vsevolodk
Copy link
Contributor

commented May 14, 2019

Add in documentation that currentSchema property can be multiple
fixes #1480

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from 13682b1 to b1564cd May 14, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 14, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 14, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 14, 2019

Codecov Report

Merging #1481 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1481      +/-   ##
============================================
+ Coverage      68.8%   68.82%   +0.02%     
- Complexity     3936     3937       +1     
============================================
  Files           179      179              
  Lines         16466    16467       +1     
  Branches       2674     2674              
============================================
+ Hits          11330    11334       +4     
+ Misses         3890     3888       -2     
+ Partials       1246     1245       -1
@davecramer

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@vsevolodk thanks for this. I think instead of "via comma" the comments should be "separated by comma's"
There's no need to change the connect.md in 94
It would be great if you added a test case.

Thanks!

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

It would be great if you added a test case.

Test case how I use several schema?

@davecramer

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Yes, we are stating that setSchema supports multiple paths. The test case would prove that it does.

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from b1564cd to b7b69de May 15, 2019

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

The test case would prove that it does.

Are you expect junit test? Or some description is enough ?

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 15, 2019

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from b7b69de to 8abe477 May 15, 2019

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

I added new tests using currentSchema property

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 15, 2019

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch 2 times, most recently from c4fb1bf to 8357b4f May 15, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 15, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 15, 2019

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@davecramer now tests are required external postgres instance.
Do you think about using https://www.testcontainers.org/ and https://www.testcontainers.org/modules/databases/postgres/ ?
We use it for our tests and it great works. We can up different configuration of postgres for several unit tests, example max_connection and etc.

@davecramer

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@vsevolodk it's been brought up before. Personally I have no use for it as at any given time I have access to multiple versions of postgres. If you want to propose it in a different PR I can look at it. We've certainly had requests for it.

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Ok, I'll check later. What about current PR about currentSchema?

@davecramer

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@vsevolodk other than moving execute into TestUtil, it looks great!

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from 8357b4f to 455d5dc May 16, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 16, 2019

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Someone else should do review?

@davecramer

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from 455d5dc to a3fb258 May 17, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented May 17, 2019

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

How to retry build? Unchanged test was failed. Tests are do rollback in db after execute? May be table creation and function can affect logical replication in another test cases?

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

@davecramer, @vlsi hi.
What about this PR? Will you merge it?

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from a3fb258 to 1f0edf9 Jun 23, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jun 23, 2019

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from 1f0edf9 to 2597799 Jun 24, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jun 24, 2019

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from 2597799 to 31a99e8 Jun 24, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jun 24, 2019

README.md Outdated Show resolved Hide resolved

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from 31a99e8 to aaa6331 Jun 24, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jun 24, 2019

@vsevolodk

This comment has been minimized.

@vlsi

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@vsevolodk , unfortunately retry does not help there.
Apparently something had happened with Trusty+oraclejdk11

Add more info about currentSchema property.
Add in documentation that currentSchema property can be multiple
and fixed behavior by tests.

@vsevolodk vsevolodk force-pushed the vsevolodk:doc/add_currentSchema_info branch from aaa6331 to ccb396d Jun 27, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jun 27, 2019

@vsevolodk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Fresh push still does not work, but previously build was success and after I changed only commas.
@vlsi @davecramer @jorsol do you have any points about this PR?
May be you can already accept and merge it?

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@vsevolodk if nobody objects overnight I'll push it tomorrow

@davecramer davecramer merged commit d96ed59 into pgjdbc:master Jun 28, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/project 68.82% (+0.02%) compared to 58804e9
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@vsevolodk vsevolodk deleted the vsevolodk:doc/add_currentSchema_info branch Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.