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

cli setup improvements #3939

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Conversation

dennybiasiolli
Copy link
Contributor

@dennybiasiolli dennybiasiolli commented Feb 19, 2024

  • cli: using fs::create_dir_all in create_migrations_directory

    To recursively create a directory and all of its parent components if they are missing.

  • cli: writing the custom migration dir to the config file

  • cli: fixing windows paths in toml strings and adding a specific test for that

@dennybiasiolli dennybiasiolli changed the title Small improvements cli setup improvements Feb 19, 2024
@dennybiasiolli dennybiasiolli force-pushed the small-improvements branch 3 times, most recently from e2bc070 to 6c5c7a5 Compare February 19, 2024 14:17
@weiznich weiznich requested a review from a team February 20, 2024 07:44
@dennybiasiolli
Copy link
Contributor Author

dennybiasiolli commented Feb 21, 2024

@weiznich rebased, to check if tests are passing now 😉

To recursively create a directory and all of its parent components if they are missing.
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR 👍 I'm generally fine with how this change is implemented and what it is supposed to change. The CI test failures on windows seem to be related to the change (and likely to the review comment left for the create_config_file function). That needs to be fixed before merging as we also need to support windows there.

diesel_cli/src/main.rs Outdated Show resolved Hide resolved
@dennybiasiolli
Copy link
Contributor Author

@weiznich I just found the reason for test errors: tests on windows are receiving an absolute path as migrations_dir and they crash on windows for that reason.
With this workaround I managed to solve the problem for tests, but a cleaner solution would be to pass a relative path when testing.
Any idea?

@weiznich
Copy link
Member

As this might also be a potential problem if someone passes an absolute path manually to the CI, I would prefer an solution that just converts the path to an path relative to the diesel.toml file instead. Maybe just call Path::strip_prefix?

@dennybiasiolli
Copy link
Contributor Author

Let's try with another solution, the problem could be that the string wrote in the .toml file must be escaped and I'm not doing that.

I'm trying with

        let migrations_dir_toml_string = format!(
            "dir = \"{}\"",
            migrations_dir.display().to_string().replace('\\', "\\\\")
        );

Workflow here: https://github.com/dennybiasiolli/diesel/actions/runs/8005969731
Fingers crossed!

@dennybiasiolli
Copy link
Contributor Author

Ok now it works! https://github.com/dennybiasiolli/diesel/actions/runs/8007224538
Let me use the same fix in this branch

@dennybiasiolli dennybiasiolli force-pushed the small-improvements branch 2 times, most recently from 1a7fef5 to d5cf157 Compare February 22, 2024 16:45
@dennybiasiolli
Copy link
Contributor Author

"STDERR: Failed to execute a database query: Identifier name 'diesel_setup_writes_migration_dir_by_arg_to_config_file_escaping_windows_paths' is too long"

This should be an easy fix 😰

@dennybiasiolli
Copy link
Contributor Author

The only failing check error now is this one:
MBERROR: The remote file either doesn't exist, is unauthorized, or is forbidden for url 'https://dev.mysql.com/get/Downloads/MySQL-8.0/mysql-8.0.31-winx64.zip'. Exception calling "Read" with "3" argument(s): "Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host."

I think it shouldn't be. problem, but feel free to re-run the workflow just to be 100% sure 😉

@weiznich weiznich added this pull request to the merge queue Feb 22, 2024
Merged via the queue into diesel-rs:master with commit 02772f5 Feb 22, 2024
47 checks passed
@dennybiasiolli dennybiasiolli deleted the small-improvements branch February 23, 2024 05:47
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.

2 participants