[-] fix sources handling in config upgrade#1274
Conversation
0xgouda
added
bug
0xgouda
force-pushed
the
fix-sources-migration
branch
from
c7b3e1e to
26c8932
Compare
0xgouda
changed the title
config upgrade
Pull Request Test Coverage Report for Build 22666439513Details
💛 - Coveralls |
| var output bytes.Buffer | ||
| opts := &Options{ | ||
| Sources: sources.CmdOpts{Sources: "/tmp/sources.yaml"}, | ||
| Metrics: metrics.CmdOpts{Metrics: "/tmp/sources.yaml"}, |
There was a problem hiding this comment.
Hey just a heads up. After the change this test becomes a duplicate of the previous test (only metrics specified as yaml - unsupported error)
| - `upgrade` | ||
|
|
||
| Upgrade the configuration and/or sink database to the latest version by applying all pending migrations. | ||
| Upgrades the metrics configuration and/or sink database to the latest version by applying all pending migrations. |
There was a problem hiding this comment.
It also update sources configuration on Postgres. Because they are stored together
There was a problem hiding this comment.
What if a user (for some weird reason) uses to separate dbs for sources and metrics?
There was a problem hiding this comment.
that's a good question. never tested in real life. I assume pgwatch will take --metrics and use it as a default database while ignoring the --sources silently
There was a problem hiding this comment.
That's why I removed source-related docs and code.
There haven't been any migrations for sources yet, so we can only ask for --metrics and --sink, and we don't care about sources for now.
And in the future, when we wish to add source migrations (if ever), we can start thinking about how we will solve this problem.
There was a problem hiding this comment.
I see your point. but sources schema can be migrated. and user must know it. the fact we do storage in the same database is our problem. if you remove source upgrade mentions, user will stuck.
There was a problem hiding this comment.
the clear way may be to make sure source writer is responsible for creating it's own tables and functions.
There was a problem hiding this comment.
Yeah, but I mean for now they are not upgradable. Once they become so in future releases, we can update docs/code and see how to handle this problem, its not urgent/necessary now.
There was a problem hiding this comment.
no. docs must tell that sources schema is upgradable. it's not important that upgrade is done with metric writer. user must know that all main database schemas are upgradable: sources, metrics, sinks.
| Upgrades the metrics configuration and/or sink database to the latest version by applying all pending migrations. | ||
| File or folder based configurations are not supported. The command will automatically detect which | ||
| databases (sources, metrics, sinks) require migrations and apply them. | ||
| databases (metrics, sinks) require migrations and apply them. |
There was a problem hiding this comment.
It will update sources stored in Postgres.
because metrics reader takes care of it because sources and metrics are stored in the same schema of the same database |
There isn't any migrations for sources and the pg sources reader/writer doesn't even implement a `Migrate()` method, so its not necessary
They are now more clear and state which flag the user needs to specify for this migration to happen correctly. This is especially useful for users who specify `--sources` without `--metrics`, then it will get used as the metrics database and on migration users will have a clue that they have to explicitly specify `--metrics` for the upgrade to happen.
0xgouda
force-pushed
the
fix-sources-migration
branch
from
779fff1 to
a35f271
Compare
|
Overtaken by: #1287 |
4 participants
don't run migrations for sources on
config upgradedoesn't even implement a
Migrate()method, so it's not necessary, and callingMigrate()on it will trigger a crash.Update
config upgradehelp messagesThey are now more clear and state which flag the user needs to specify
for this migration to happen correctly.
This is especially useful for users who specify
--sourceswithout--metrics,then it will get used as the metrics database and on migration users will have a clue that
they have to explicitly specify
--metricsfor the upgrade to happen.Closes: #1273