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

Resolves #647 - Fixes typos in Mongo advisory locking parameters #648

Merged
merged 8 commits into from Nov 5, 2021

Conversation

SJrX
Copy link
Contributor

@SJrX SJrX commented Oct 27, 2021

make-test was failing a bunch for me on neo4j, and mssql, not sure if it's just my environment however. I can investigate if you think it's a risk.

This is for #647

@coveralls
Copy link

@coveralls coveralls commented Oct 27, 2021

Pull Request Test Coverage Report for Build 1424405064

  • 12 of 15 (80.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 57.729%

Changes Missing Coverage Covered Lines Changed/Added Lines %
database/mongodb/mongodb.go 12 15 80.0%
Totals Coverage Status
Change from base Build 1411044316: 0.04%
Covered Lines: 3731
Relevant Lines: 6463

💛 - Coveralls

Copy link
Member

@dhui dhui left a comment

Thanks for the find and fix!
I can't believe I missed this... 🤦

Luckily this exact typo only occurs here!

$ git grep -l timout
database/mongodb/README.md
database/mongodb/mongodb.go


if lockTimeout == "" {
// The initial release had a typo for this argument but for backwards compatibility sake, we will keep supporting it.
lockTimeout = unknown.Get("x-advisory-lock-timout-interval")
Copy link
Member

@dhui dhui Oct 30, 2021

Choose a reason for hiding this comment

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

Thanks for preserving the old behavior! Let's return an error explaining that only one should be specified if both are specified.

Copy link
Contributor Author

@SJrX SJrX Oct 30, 2021

Choose a reason for hiding this comment

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

Is that really important. I am on vacation for a few days, but the only way that would happen is if someone looked in the source code to see this behavior as we aren't documenting both, so I'm not sure if it is the end of the world if we just use the correct one.

Copy link
Member

@dhui dhui Oct 31, 2021

Choose a reason for hiding this comment

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

This fix isn't urgent so can wait until you're back from vacation. Or I can make the change (based off of your change). I'd like to support existing users in a backwards compatible manner but also eliminate any confusion if both parameters are specified.

Copy link
Contributor Author

@SJrX SJrX Nov 4, 2021

Choose a reason for hiding this comment

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

Hows that @dhui ?

Copy link
Member

@dhui dhui left a comment

Thanks for addressing my feedback!

A couple minor things to fix and and we should be good to merge.

CONTRIBUTING.md Outdated
@@ -11,7 +11,7 @@
1. Write awesome code ...
1. `make test` to run all tests against all database versions
1. Push code and open Pull Request

:wq
Copy link
Member

@dhui dhui Nov 5, 2021

Choose a reason for hiding this comment

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

vim!

Copy link
Contributor Author

@SJrX SJrX Nov 5, 2021

Choose a reason for hiding this comment

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

Fixed

ErrNilConfig = fmt.Errorf("no config")
ErrNoDatabaseName = fmt.Errorf("no database name")
ErrNilConfig = fmt.Errorf("no config")
ErrTypoAndNotNonTypoUsed = fmt.Errorf("both x-advisory-lock-timeout-interval and x-advisory-lock-timout-interval were specified")
Copy link
Member

@dhui dhui Nov 5, 2021

Choose a reason for hiding this comment

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

Since this is an exported specific error, let's make the name specific as well. e.g. something along the lines of: ErrLockTimeoutConfigConflict

Copy link
Contributor Author

@SJrX SJrX Nov 5, 2021

Choose a reason for hiding this comment

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

Done

@SJrX SJrX requested a review from dhui Nov 5, 2021
dhui
dhui approved these changes Nov 5, 2021
Copy link
Member

@dhui dhui left a comment

Thanks again for the find, fix, and addressing of feedback!

@dhui dhui merged commit 4ba6957 into golang-migrate:master Nov 5, 2021
8 checks passed
kumaya added a commit to infobloxopen/migrate that referenced this issue Nov 26, 2021
* 'master' of github.com:golang-migrate/migrate: (418 commits)
  Run gofmt on internal build dir
  go mod tidy
  Resolves golang-migrate#647 - Fixes typos in Mongo advisory locking parameters (golang-migrate#648)
  Bump version of autorest/adal
  Set syntax highlighting for pkger example
  Add pkger to README
  change github auth to use oauth token instead of basic.
  Use the recommended v4 in mysql README
  go mod tidy
  fix test
  Delete all rows
  Use ParseBool
  Support for AWS Keyspaces
  Update gosnowflake from v1.4.3 to v1.6.3
  Update docker client usage with breaking change
  Update dktest to v0.3.7 to fix security warnings
  revert binary file location change in docker image
  fix source/file driver
  Update build constraints
  Update golangci-lint config
  ...
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.

None yet

3 participants