Skip to content

Add migration parameters #3805

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

Closed
wants to merge 4 commits into from
Closed

Conversation

krlohnes
Copy link

Does your PR solve an issue?

closes #3178

Is this a breaking change?

No, this is not a breaking change. The feature is gated by a comment annotation that surrounds the parts of the migration to be parameterized.

Added migration parameters in a similar fashion to goose's env var
migration parameters. This still needs some tests and documentation.
I wanted to get a proposal for a solution up for folks to take a look
at.
@abonander
Copy link
Collaborator

Sorry, I just don't like the approach here.

  • Some users already don't like the reliance on environment variables.
  • It can easy to forget to specify environment variables which would lead to an error at runtime.
  • Users might expect more interaction with migrate!() or may even expect variables to be expanded at compile-time (or at least to have that option).
  • It's impossible to programmatically set these variables without using env::set_var(), which is not thread-safe on most platforms.
  • Using the regex crate to find the start and end of substitution-enabled sections is kind of overkill.

Because this is unlikely to be merged without significant changes, I'm just going to close. Let's take the discussion back to #3178 where I'll give my thoughts on what a proper solution should look like.

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.

Add migration parameters or the possibility of passing migrations by string
2 participants