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.

Sign up for GitHub

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

Jump to bottom

Removal of files option of the cli + dep upgrade + cs #175

Merged
merged 5 commits into from
Sep 18, 2021
Merged

Removal of files option of the cli + dep upgrade + cs #175

merged 5 commits into from
Sep 18, 2021

Conversation

omid
Copy link
Contributor

@omid omid commented Sep 13, 2021

Fixes #174

jxs
jxs requested changes Sep 15, 2021
View reviewed changes
Copy link
Member

@jxs jxs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this. There are still refferences to the files subcommand and the cli tests still use it also, that's why they are failing.
Due to what we discussed on #170 it probably doesn't make sense to support .rs migration files here, so we should introduce differentiation between just .sql files and both of them similar to what we had here to be called on just .sql files for the cli case, sorry I missed it on #170.
What do you think?

@omid
Copy link
Contributor Author

omid commented Sep 17, 2021

How come I forgot to touch the logic :D stupid me.

Anyway, about what you said. Sorry, I didn't get it, why should we only run .sql files with CLI? Or you mean something else?

@omid
Copy link
Contributor Author

omid commented Sep 17, 2021 •
edited
Loading

OK, I think I understood what you mean. We should add the rs support to cli. I'm on it.

@jxs
Copy link
Member

jxs commented Sep 18, 2021 •
edited
Loading

Hi, no worries 😀 meanwhile I have addressed that issue while working on #177, see jxs@fa28ffc and feel free to rebase against main

EDIT: turns out no need, so I'll just review :)

jxs
jxs approved these changes Sep 18, 2021
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM thanks! 🎆

refinery_cli/src/migrate.rs
config_location: &str,
grouped: bool,
divergent: bool,
missing: bool,
env_var_opt: Option<&str>,
arg: &ArgMatches,
path: &str,
Copy link
Member

Choose a reason for hiding this comment

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

thanks for this improvement 👌

@jxs jxs merged commit 273eb2f into rust-db:main Sep 18, 2021
@omid
Copy link
Contributor Author

omid commented Sep 18, 2021

OK, cool 👍🏼

@omid omid deleted the remove_files_from_cli branch September 18, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jxs jxs jxs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

What is the purpose of "files" sub-command
2 participants
@omid @jxs