refactor: move migrations to gobuffalo/fizz #1775
Merged
Conversation
This file contains 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
# Conflicts: # docs/api.swagger.json # go.sum # internal/httpclient/models/accept_login_request.go # internal/httpclient/models/consent_request.go # internal/httpclient/models/consent_request_session.go # internal/httpclient/models/flush_inactive_o_auth2_tokens_request.go # internal/httpclient/models/json_web_key_set.go # internal/httpclient/models/json_web_key_set_generator_request.go # internal/httpclient/models/o_auth2_token_introspection.go
|
@aeneasr hey this is kinda working now |
|
Good job! Let's discuss this on slack/in a hangout. Ping me when you're online :) |
|
To do:
|
# Conflicts: # go.mod
# Conflicts: # .schema/api.swagger.json # consent/sql_migration_files.go # internal/httpclient/models/completed_request.go # internal/httpclient/models/generic_error.go # internal/httpclient/models/health_not_ready_status.go # internal/httpclient/models/json_web_key_set.go # internal/httpclient/models/json_web_key_set_generator_request.go # internal/httpclient/models/login_request.go # internal/httpclient/models/o_auth2_token_introspection.go # internal/httpclient/models/previous_consent_session.go # internal/httpclient/models/reject_request.go # internal/httpclient/models/userinfo_response.go # internal/httpclient/models/well_known.go # oauth2/sql_migration_files.go
aeneasr
commented
Apr 22, 2020
client/manager_test.go
Outdated
| log.Fatalf("Could not connect to database: %v", err) | ||
| } | ||
|
|
||
| func getManager(t *testing.T, url string) Manager { |
There was a problem hiding this comment.
Move this to the internal package
| @@ -84,21 +84,9 @@ func TestManagers(t *testing.T) { | |||
| regs["memory"] = internal.NewRegistry(conf) | |||
|
|
|||
| if !testing.Short() { | |||
| var p, m, c *sqlx.DB | |||
| dockertest.Parallel([]func(){ | |||
driver/registry_sql.go
Outdated
| table.Append([]string{m.db.DriverName(), names[component], plan.Id + ".sql", fmt.Sprintf("%d", k), up}) | ||
| } | ||
| } | ||
| if err := c.Open(); err != nil { |
There was a problem hiding this comment.
This must be retried, see connection.GetDatabaseRetry(time.Second*5, time.Minute*5)!
persistence/sql/migratest/helpers.go
Outdated
| } | ||
|
|
||
| if err := tx.RawQuery(string(data)).Exec(); err != nil { | ||
| log.Print(mf.Version) |
There was a problem hiding this comment.
Suggested change
| log.Print(mf.Version) | |
| t.Logf(mf.Version) |
| return | ||
| } | ||
|
|
||
| for db, connect := range map[string]func(*testing.T) *pop.Connection{ |
There was a problem hiding this comment.
Start these in parallel using dockertest.Parallel
| }) | ||
| } | ||
|
|
||
| // TODO this is very stupid and should be replaced as soon the manager uses pop |
There was a problem hiding this comment.
Suggested change
| // TODO this is very stupid and should be replaced as soon the manager uses pop | |
| // TODO https://github.com/ory/hydra/issues/1815 | |
| // this is very stupid and should be replaced as soon the manager uses pop |
| @@ -0,0 +1 @@ | |||
| INSERT INTO hydra_client (id, client_name, client_secret, redirect_uris, grant_types, response_types, scope, owner, policy_uri, tos_uri, client_uri, logo_uri, contacts, public) VALUES ('client-0001', 'Client 0001', 'secret-0001', 'http://redirect/0001_1', 'grant-0001_1', 'response-0001_1', 'scope-0001', 'owner-0001', 'http://policy/0001', 'http://tos/0001', 'http://client/0001', 'http://logo/0001', 'contact-0001_1', true); | |||
There was a problem hiding this comment.
Note to self: figure out how to review this
persistence/sql/persister.go
Outdated
|
|
||
| // this type is copied from sql-migrate to remove the dependency | ||
| type OldMigrationRecord struct { | ||
| Id string `db:"id"` |
There was a problem hiding this comment.
Suggested change
| Id string `db:"id"` | |
| ID string `db:"id"` |
|
The demo fails with: To run the demo, do: |
# Conflicts: # internal/httpclient/models/accept_consent_request.go # internal/httpclient/models/consent_request_session.go # internal/httpclient/models/flush_inactive_o_auth2_tokens_request.go # internal/httpclient/models/health_status.go # internal/httpclient/models/json_web_key.go # internal/httpclient/models/json_web_key_set.go # internal/httpclient/models/logout_request.go # internal/httpclient/models/o_auth2_token_introspection.go # internal/httpclient/models/open_id_connect_context.go # internal/httpclient/models/previous_consent_session.go # internal/httpclient/models/reject_request.go # internal/httpclient/models/userinfo_response.go
# Conflicts: # internal/httpclient/models/consent_request.go # internal/httpclient/models/flush_inactive_o_auth2_tokens_request.go # internal/httpclient/models/generic_error.go # internal/httpclient/models/health_not_ready_status.go # internal/httpclient/models/health_status.go # internal/httpclient/models/json_web_key_set.go # internal/httpclient/models/json_web_key_set_generator_request.go # internal/httpclient/models/logout_request.go # internal/httpclient/models/o_auth2_client.go # internal/httpclient/models/previous_consent_session.go # internal/httpclient/models/userinfo_response.go
# Conflicts: # client/sql_migration_files.go # consent/sql_migration_files.go # go.sum # internal/httpclient/models/completed_request.go # internal/httpclient/models/health_not_ready_status.go # internal/httpclient/models/json_web_key_set_generator_request.go # internal/httpclient/models/login_request.go # internal/httpclient/models/o_auth2_client.go # internal/httpclient/models/oauth2_token_response.go # internal/httpclient/models/open_id_connect_context.go # internal/httpclient/models/previous_consent_session.go # internal/httpclient/models/userinfo_response.go # jwk/sql_migration_files.go # oauth2/sql_migration_files.go
|
@aeneasr quickstart is now working for me and all tests pass, ready for review |
aeneasr
commented
Apr 27, 2020
| @@ -27,14 +32,13 @@ test-resetdb: | |||
| docker rm -f hydra_test_database_mysql || true | |||
| docker rm -f hydra_test_database_postgres || true | |||
| docker rm -f hydra_test_database_cockroach || true | |||
| docker run --rm --name hydra_test_database_mysql -p 3444:3306 -e MYSQL_ROOT_PASSWORD=secret -d mysql:5.7 | |||
internal/driver.go
Outdated
Comment on lines
163
to
175
| wg.Add(3) | ||
| go func() { | ||
| CleanAndMigrate(pg)(t) | ||
| wg.Done() | ||
| }() | ||
| go func() { | ||
| CleanAndMigrate(mysql)(t) | ||
| wg.Done() | ||
| }() | ||
| go func() { | ||
| CleanAndMigrate(crdb)(t) | ||
| wg.Done() | ||
| }() |
There was a problem hiding this comment.
Suggested change
| wg.Add(3) | |
| go func() { | |
| CleanAndMigrate(pg)(t) | |
| wg.Done() | |
| }() | |
| go func() { | |
| CleanAndMigrate(mysql)(t) | |
| wg.Done() | |
| }() | |
| go func() { | |
| CleanAndMigrate(crdb)(t) | |
| wg.Done() | |
| }() | |
| dbs := []*driver.RegistrySQL{pg, mysql, crdb} | |
| wg.Add(len(dbs)) | |
| for _, db := range dbs { | |
| go func(db *driver.RegistrySQL) { | |
| defer wg.Wait() | |
| CleanAndMigrate(db)(t) | |
| }(db) | |
| } | |
| wg.Wait() |
internal/driver.go
Outdated
Comment on lines
163
to
175
| wg.Add(3) | ||
| go func() { | ||
| CleanAndMigrate(pg)(t) | ||
| wg.Done() | ||
| }() | ||
| go func() { | ||
| CleanAndMigrate(mysql)(t) | ||
| wg.Done() | ||
| }() | ||
| go func() { | ||
| CleanAndMigrate(crdb)(t) | ||
| wg.Done() | ||
| }() |
There was a problem hiding this comment.
Suggested change
| wg.Add(3) | |
| go func() { | |
| CleanAndMigrate(pg)(t) | |
| wg.Done() | |
| }() | |
| go func() { | |
| CleanAndMigrate(mysql)(t) | |
| wg.Done() | |
| }() | |
| go func() { | |
| CleanAndMigrate(crdb)(t) | |
| wg.Done() | |
| }() | |
| dbs := []*driver.RegistrySQL{pg, mysql, crdb} | |
| wg.Add(len(dbs)) | |
| for _, db := range dbs { | |
| go func(db *driver.RegistrySQL) { | |
| defer wg.Wait() | |
| CleanAndMigrate(db)(t) | |
| }(db) | |
| } |
| .PHONY: test-legacy-migrations | ||
| test-legacy-migrations: | ||
| make test-resetdb | ||
| source scripts/test-env.sh && go test -tags legacy_migration_test -failfast -timeout=20m ./internal/fizzmigrate |
There was a problem hiding this comment.
isn't make sqlbin missing here?
There was a problem hiding this comment.
hm right I thought we don't need it as the old migrations should not change but if they change we should make sqlbin here
driver/registry.go
Outdated
| @@ -56,6 +60,7 @@ func MustNewRegistry(c configuration.Provider) Registry { | |||
| } | |||
|
|
|||
| func NewRegistry(c configuration.Provider) (Registry, error) { | |||
| fmt.Printf("got dsn '%s'\n", c.DSN()) | |||
There was a problem hiding this comment.
Suggested change
| fmt.Printf("got dsn '%s'\n", c.DSN()) |
driver/registry.go
Outdated
| @@ -1,9 +1,12 @@ | |||
| package driver | |||
|
|
|||
| import ( | |||
| "fmt" | |||
go.mod
Outdated
| @@ -59,3 +60,5 @@ require ( | |||
| golang.org/x/tools v0.0.0-20200313205530-4303120df7d8 | |||
| gopkg.in/square/go-jose.v2 v2.5.0 | |||
| ) | |||
|
|
|||
| replace github.com/gobuffalo/pop/v5 => github.com/zepatrik/pop/v5 v5.0.12-0.20200418094557-850d559d8122 | |||
There was a problem hiding this comment.
This is still to do @zepatrik
internal/driver.go
Outdated
| @@ -78,6 +84,101 @@ func NewRegistrySQL(c *configuration.ViperProvider, db *sqlx.DB) *driver.Registr | |||
| return r.(*driver.RegistrySQL) | |||
| } | |||
|
|
|||
| func NewRegistryPop(t *testing.T, url string) *driver.RegistrySQL { | |||
There was a problem hiding this comment.
Suggested change
| func NewRegistryPop(t *testing.T, url string) *driver.RegistrySQL { | |
| func NewRegistrySQL(t *testing.T, url string) *driver.RegistrySQL { |
oauth2/fosite_store_test.go
Outdated
| return db | ||
| var registries = make(map[string]driver.Registry) | ||
| var cleanRegistries = func(*testing.T) { | ||
| fmt.Printf("\n\nsetting memory reg\n\n\n") |
There was a problem hiding this comment.
Suggested change
| fmt.Printf("\n\nsetting memory reg\n\n\n") |
# Conflicts: # .schema/api.swagger.json # internal/httpclient/models/generic_error.go # internal/httpclient/models/json_web_key_set.go # internal/httpclient/models/json_web_key_set_generator_request.go # internal/httpclient/models/login_request.go # internal/httpclient/models/logout_request.go # internal/httpclient/models/o_auth2_token_introspection.go # internal/httpclient/models/previous_consent_session.go # internal/httpclient/models/reject_request.go # internal/httpclient/models/userinfo_response.go # internal/httpclient/models/well_known.go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Related issue
Proposed changes
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further comments