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

Open/Extend API to be less restrictive #25

Open
Kafkalasch opened this issue Aug 9, 2023 · 4 comments
Open

Open/Extend API to be less restrictive #25

Kafkalasch opened this issue Aug 9, 2023 · 4 comments
Assignees

Comments

@Kafkalasch
Copy link
Contributor

Kafkalasch commented Aug 9, 2023

The provided API / public methods are a bit cumbersome to use.

We want to use the library within our application AND use migration files. Unfortunately there does not seem to be an official way of how that is supported.

Ho we use it right now:

//go:linkname migrations github.com/deusdat/arangomigo.migrations
func migrations(migrationPath []string) ([]arangomigo.PairedMigrations, error)

//go:linkname perform github.com/deusdat/arangomigo.perform
func perform(ctx context.Context, c arangomigo.Config, pms []arangomigo.PairedMigrations) error

func DoMigration() error {
        config := arangomigo.Config{Db: dbName, Endpoints: []string{dbUrl}, MigrationsPath: []string{directory}}
	pms, _ := migrations(config.MigrationsPath)
	_ = perform(ctx, config, pms)
}

This allows the a simple usage of this library within our code base. The drawback is that this is only possible in this hacky way of using internal, non exported functions.

I would like to suggest to export those functions so they can be used that easily directly.

@Kafkalasch
Copy link
Contributor Author

But obviously this would lead to a larger API that needs to be maintained, so not sure if you want this

@virmundi virmundi self-assigned this Aug 11, 2023
@virmundi
Copy link
Contributor

I'm ok with widening the API a bit. https://github.com/deusdat/arangomigo/compare/feature-ISSUE-25-Open-API?expand=1

The reason this wasn't exposed before is I'm not a huge fan of embedded migrations. Working with Flyway, for example, if two instances of the app attempt to migrate at once, it can cause weird race conditions. The same could happen here. Instead I prefer to have a step in my deploy that invokes the migrator. If it succeeds, the app deploys. If it doesn't, the migrator rolls back the changes and my prod environment is not really the wiser.

Let me know if that branch does what you want.

@Kafkalasch
Copy link
Contributor Author

Ah, yes, I see your point.

And yes, that branch would solve/fix our issue. thank you

@hiendaovinh
Copy link

Exposing migrate (e.g. Migrate) would be great so we don't have to create config, marshal it, write it to a temp file and pass the path to you.

https://github.com/deusdat/arangomigo/blob/430e8a1c57f5a5c3c1083d411903ee44f93a58c2/arangomigo.go#L30C6-L30C13

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

No branches or pull requests

3 participants