chore: add context.Context everywhere #1132
Conversation
| @@ -122,10 +123,11 @@ Database drivers: `+strings.Join(database.List(), ", ")+"\n", createUsage, gotoU | |||
| // initialize migrate | |||
| // don't catch migraterErr here and let each command decide | |||
| // how it wants to handle the error | |||
| migrater, migraterErr := migrate.New(*sourcePtr, *databasePtr) | |||
| ctx := context.Background() | |||
There was a problem hiding this comment.
This is the only place where passing the context is clearly an improvement.
Changing every other place without an explicit use case seems excessive 🤷
There was a problem hiding this comment.
I have an explicit use case: adding OpenTelemetry support to this project so that the progress can be followed in the observability tool of your choice with additional information on what's happening and what exactly went wrong in case of an error.
This relies heavily on passing around context, but I don't want to spend more time on it (outside of my internal WIP) if this particular change isn't viable or desired.
I acknowledge that on its own it's pretty useless and mostly adds noise to the existing code while breaking backward compatibility. 🤷
joschi
force-pushed
the
chore-context-everywhere
branch
3 times, most recently
from
4ff8b07 to
d8b0199
Compare
joschi
force-pushed
the
chore-context-everywhere
branch
from
d8b0199 to
6fe5f52
Compare
joschi
force-pushed
the
chore-context-everywhere
branch
2 times, most recently
from
681c89b to
65ef417
Compare
joschi
force-pushed
the
chore-context-everywhere
branch
from
65ef417 to
e9f5c08
Compare
|
@dhui I am aware that this is a breaking change in migrate's API (although a quite mechanic one). Would you still be interested in this as a precursor to introducing tracing support in migrate (potentially starting migrate/v5)? |
This PR changes the internal API of the
database.Driverandsource.Driverinterfaces so that every function accepts acontext.Contextto enable future use cases which require passing around data between these functions.There are no other functional changes in this PR except for fixing the Redshift tests via #1128.