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

improved cockroach configuration and fixed bugs on connection details #271

Merged
merged 3 commits into from
Oct 16, 2018

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Oct 15, 2018

Hello, @markbates and all,
While I tried to improve configuration support for cockroach, I also found some bugs on connection details. This pull request with a single commit covers follows:

original problems are:

  • If cockroach was configured as secure mode, the only way to configure the connection is using URL.
  • If the user uses URL for cockroach, database creation/drop is not working because it uses generated URL with connection details. (it has both parsing error and secure mode problem)
  • If the user setup connection for SQLite using both dialect and url, it cannot detect Database.

solved problems are:

  • Now user can configure SQLite with both dialect and url. (test case added)
  • User can configure a connection for Cockroach with both a list of options and url. (test case added)
  • Autogenerated URL to create/drop database now support secure mode. (test case added for URL builder)

while fixing it,

  • A bug was founded on regexp for checking scheme part of the URL and fixed. (test case improved)
  • The function Finalize() is somewhat hard to read because it is somewhat long and contains both URL handling and default value handling, so I just break it into three parts, overrideWithURL() for URL handling, overrideWithMySQLURL() for URL handling for MySQL, and Finalize() for main and default value handling.

IMHO, the last part of the fix, splitting the Finalize(), is necessary because I think the bugs come from this hard readability.

Please check this PR.

@sio4 sio4 requested a review from a team as a code owner October 15, 2018 12:38
@sio4
Copy link
Member Author

sio4 commented Oct 15, 2018

Also, fixed a bug of buffalo db schema dump is not working in secure mode with error message like:

Error: getBasicMetadata: relation pop_test.public. does not exist

@stanislas-m stanislas-m merged commit e9bd038 into gobuffalo:development Oct 16, 2018
@sio4
Copy link
Member Author

sio4 commented Oct 16, 2018

Thanks. It will help me a lot, and the others try to use cockroach with pop & buffalo.

@sio4 sio4 deleted the cockroach branch October 17, 2018 07:59
mclark4386 pushed a commit that referenced this pull request Oct 21, 2018
…#271)

* improved cockroach configuration and fixed bugs on connection details

* oops! didn't remove debugging log

* bugfix: blank argument also affect as argument, in secure mode
stanislas-m pushed a commit that referenced this pull request Oct 26, 2018
…#271)

* improved cockroach configuration and fixed bugs on connection details

* oops! didn't remove debugging log

* bugfix: blank argument also affect as argument, in secure mode
stanislas-m pushed a commit that referenced this pull request Oct 29, 2018
…#271)

* improved cockroach configuration and fixed bugs on connection details

* oops! didn't remove debugging log

* bugfix: blank argument also affect as argument, in secure mode
@sio4
Copy link
Member Author

sio4 commented Oct 29, 2018

Hi @stanislas-m
What is happen? all content of this PR was gone away.
5dcc891 just contains shoulders.md.

stanislas-m pushed a commit that referenced this pull request Nov 3, 2018
* Reset to development

* Query in support with slice args. (#267)

* [Fix] issue when using IN more than once '?' wildcards

* slices with in

* Fix some code style issues

* Trim the Readme and add SHOULDERS (#268)

* The lastest docs are on the website, and keeping them here mislead users.
* Let's use SHOULDERS here too!

* improved cockroach configuration and fixed bugs on connection details (#271)

* improved cockroach configuration and fixed bugs on connection details

* oops! didn't remove debugging log

* bugfix: blank argument also affect as argument, in secure mode

* Should fix not being able to use existing models in eager create associations

* Should fix not being able to use existing models in eager create associations

* added test to show what happens with partial update of associated object

* added test to show what happens with partial update of associated object

* *saving spot so I can rebase, but it's broken this commit...

* *fixed bug and added a debug_test function to test script if needed

* *linting and some copy paste I thought I fixed

* *it may be done... going to improve testing to make sure it's good and solid

* *best to put the comment in the right place if it's going to remind me properly...

* *refactoring, bug fix, and better testing

* *remove debug logging

* *don't need that it would be doubling the call up since flat create handles it and that was called earlier

* *improved docs and test coverage

* *should fix requested changes (cleaned up the code a bit too)

* *shouldn't be checking empty incorrectly anymore

* *removed some un-needed code
*added some docs
*IsZeroOf now does a DeepEqual explictly

* Reset to development

* Query in support with slice args. (#267)

* [Fix] issue when using IN more than once '?' wildcards

* slices with in

* Fix some code style issues

* Trim the Readme and add SHOULDERS (#268)

* The lastest docs are on the website, and keeping them here mislead users.
* Let's use SHOULDERS here too!

* improved cockroach configuration and fixed bugs on connection details (#271)

* improved cockroach configuration and fixed bugs on connection details

* oops! didn't remove debugging log

* bugfix: blank argument also affect as argument, in secure mode

* Deprecate Left and Right InnerJoins in favor of one InnerJoin. (#275)

* Deprecate Left and Right InnerJoins in favor of one InnerJoin.

* Add oncer Deprecation notices to LeftInnerJoin and RightInnerJoin

* *should fix tests ('zero' UUIDs not getting seem as zero because they are converted to non-empty strings by ID())

* *updated update statement with more docs and to use sqlx.In

* Improved tests

* *couple of stupid ones

* *removed leftover import from conflict resolve merge

* *should fix checking an empty UUID in eagerCreate and validateAndOnlyCreate

* *fix some merge mistakes and a fix

* requested changes

* Fix merge issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants