-
Notifications
You must be signed in to change notification settings - Fork 334
Add :migrations_paths repo configuration option
#695
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
Open
pnezis
wants to merge
2
commits into
elixir-ecto:master
Choose a base branch
from
pnezis:migrations-paths-config
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should actually move the logic of
migrations_pathhere and reimplementmigrations_pathashd(migrations_paths(...)), adding a note it is now discouraged. Otherwise you can have code usingmigrations_pathandmigrations_pathsand working with two completely set of directories.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept
migrations_pathunchanged to ensure backwards compatibility. If we callhd(migrations_paths(...))and the:migrations_pathsis set with more than one paths then it will only return the first one.Should we raise in such cases? e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raising is good! But please use pattern matching! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, in order to handle the optional
directoryargument expected bymigrations_path/1I used anoptsinmigrations_paths/1There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I would prefer to not expose the directory option, since it doesn't work with the configuration, so perhaps we have to refactor this into a private function where we keep the directory hidden.
I believe the directory though was added for people who want to have additional migrations directories, but not plug them all by default. So this pull request has the unfortunate side-effect of making those cases harder. :( And now I am not sure how to move forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#245 introduced this.
If you ask me, since the mix tasks support passing multiple migrations paths, we should only have
migrations_paths. We could keepmigrations_path/1as it is for backwards compatibility or soft deprecate is.Currently if you call
migrations_pathwith a different directory then you will need to pass it explicitly in theecto.migrate --migrations-pathin order to be applied.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's how some projects tackle it indeed. In production you run them at distinct moments, so you don't want the additional path as default. And then you have mix tasks/aliases so you don't specify all of them:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I could bring its implementation back and have something like:
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main problem is when you have multiple repos:
with the suggested change
currently
you need to create an for each repo alias and re-alias
ecto.migrateor create another alias in order to workwhich works but is not convenient and prone to errors, e.g. when you want to add a new repo, or use the same repo in multiple apps