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 migration creation + Windows fix #352

Merged
merged 19 commits into from
Mar 10, 2020

Conversation

r3code
Copy link
Contributor

@r3code r3code commented Mar 4, 2020

This PR incorporates all the features added by PR #312 to migrate tool, so it supersedes it.
What the difference?

  1. Windows OS dir path issue fixed
  2. CreateCmd tests fixed for Windows and Linux (dir invalid case especially)
  3. each *Cmd() in commands.go now returns an error on fail and do not use log.fatalErr(err).
  4. migrate create now prints out absolute path to created files.
  5. Added comments requested by @dhui
  6. migrate usage docs updated.

13k and others added 15 commits December 4, 2019 04:46
* Validates migration version on creation, to avoid creation of duplicated versions
* Uses `os.OpenFile` with `O_CREATE|O_EXCL` to create files to avoid file collisions
* Uses `filepath.Join` to concatenate paths, making `cleanPath()` not necessary
* Prints generated filenames
* Fixes golang-migrate#238
* Supersedes golang-migrate#250
Better for Windows OS when specify -dir as /subdir, user will see C:/subdir/0001_name.up.sql rather than /subdir/0001_name.up.sql
abs path tests fails because filepath.IsAbs() treats `/subdir` path as invalid abs path at windows when drive letter is not present
Better for Windows OS systems where `/path` can be interpreted in different ways depending on working dir
OS Windows has different interpretation of `/path`, it depends on working dir.
If working dir D:\test it interprets `/path` as `D:\path`
Linux OS has less restriction on a filepath than Windows, so path invalid in windows is perfectly valid for Linux. The only invalid dir name in Linux is one ending with null string terminator (\000)
@r3code
Copy link
Contributor Author

r3code commented Mar 4, 2020

@dhui can you conduct a review?

@coveralls
Copy link

coveralls commented Mar 4, 2020

Pull Request Test Coverage Report for Build 715

  • 66 of 122 (54.1%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 53.191%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/cli/main.go 0 22 0.0%
internal/cli/commands.go 66 100 66.0%
Files with Coverage Reduction New Missed Lines %
internal/cli/commands.go 1 61.27%
Totals Coverage Status
Change from base Build 713: 0.7%
Covered Lines: 2559
Relevant Lines: 4811

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 709

  • 66 of 122 (54.1%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 53.191%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/cli/main.go 0 22 0.0%
internal/cli/commands.go 66 100 66.0%
Files with Coverage Reduction New Missed Lines %
internal/cli/commands.go 1 61.27%
Totals Coverage Status
Change from base Build 708: 0.7%
Covered Lines: 2559
Relevant Lines: 4811

💛 - Coveralls

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks so much for addressing the issues in the other PR. Having all of the internal cli command methods return errors is so much cleaner!

Just a few minor fixes and this PR is ready to be merged

internal/cli/commands_test.go Outdated Show resolved Hide resolved
internal/cli/main.go Outdated Show resolved Hide resolved
@dhui dhui merged commit 8e142df into golang-migrate:master Mar 10, 2020
FPiety0521 pushed a commit to FPiety0521/Golang-SQL that referenced this pull request May 24, 2023
* CLI: Improve migration creation.

* Validates migration version on creation, to avoid creation of duplicated versions
* Uses `os.OpenFile` with `O_CREATE|O_EXCL` to create files to avoid file collisions
* Uses `filepath.Join` to concatenate paths, making `cleanPath()` not necessary
* Prints generated filenames
* Fixes #238
* Supersedes #250

* CLI: change `createCmd` to return error and accept `print` parameter

* feat: print out an abs path

Better for Windows OS when specify -dir as /subdir, user will see C:/subdir/0001_name.up.sql rather than /subdir/0001_name.up.sql

* test: fixed abs path test fail for OS Windows

abs path tests fails because filepath.IsAbs() treats `/subdir` path as invalid abs path at windows when drive letter is not present

* feat: print absolute path for created files

Better for Windows OS systems where `/path` can be interpreted in different ways depending on working dir

* test: corrected tests for OS Windows

OS Windows has different interpretation of `/path`, it depends on working dir.
If working dir D:\test it interprets `/path` as `D:\path`

* test: fixed `dir invalid` test

Linux OS has less restriction on a filepath than Windows, so path invalid in windows is perfectly valid for Linux. The only invalid dir name in Linux is one ending with null string terminator (\000)

* refac(cli): *Cmd() now returns an error and not uses log.fatalErr(err)

* docs: added godoc, migarate usage updated

* refac(cli): code refactored

* refac: removed unnecessary path covert

* docs: comment added

* test: fixed code review issue, noErrorExpected var removed

golang-migrate/migrate#352 (comment)

* docs: fixed godoc

Co-authored-by: Kiyoshi '13k' Murata <kbmurata@gmail.com>
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

4 participants