-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add a way to set postgres role when executing migrations #226
Conversation
In same cases we want to set a specific role when executing migrations, so the ownerhsip of the created/updated objects is different from the pgroll user (storing pgroll state). This change allows to set a role that will be set in the connection executing migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is worth adding a test for this, to ensure that objects are created with the expected role when the option is set?
I added some tests, let me know what you think! |
pkg/testutils/util.go
Outdated
@@ -113,7 +113,7 @@ func WithStateAndConnectionToContainer(t *testing.T, fn func(*state.State, *sql. | |||
fn(st, db) | |||
} | |||
|
|||
func WithMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t *testing.T, schema string, lockTimeoutMs int, fn func(mig *roll.Roll, db *sql.DB)) { | |||
func WithMigratorInSchemaConnectionToContainerWithOptions(t *testing.T, schema string, opts []roll.Option, fn func(mig *roll.Roll, db *sql.DB)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To follow the pattern of the other With*
helper functions this could be called:
WithMigratorInSchemaAndConnectionToContainerWithOptions
pkg/testutils/util.go
Outdated
os.Exit(1) | ||
} | ||
|
||
// create handy role for tets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo tets->tests
@@ -153,7 +175,7 @@ func WithMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t *testing.T, s | |||
t.Fatal(err) | |||
} | |||
|
|||
mig, err := roll.New(ctx, connStr, schema, lockTimeoutMs, st) | |||
mig, err := roll.New(ctx, connStr, schema, st, opts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than create a role called pgroll
in SharedTestMain
would it better to create a role with the role name specified in opts
here?
The way it works now is that the only role option name that will actually work is pgroll
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these opts are part of the roll package, should I make Options public for this? happy to do the change but a bit on the fence on the benefit and resulting interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to leave the hard-coded role creation of thepgroll
role as-is in this case then 👍
pkg/testutils/util.go
Outdated
os.Exit(1) | ||
} | ||
|
||
// create handy role for tets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in another comment it might be better not to create a role here but do it in the test helper function instead.
In same cases we want to set a specific role when executing migrations, so the ownerhsip of the created/updated objects is different from the pgroll user (storing pgroll state). This change allows to set a role that will be set in the connection executing migrations.