fix: sink-only schema upgrade now works (#1114) #1138
Conversation
|
so afaik the config init command handles sources/metrics/sinks independently, but upgrade didn't ?. Is there a reason upgrade originally required both? Just curious |
pashagolub
added
bug
internal/cmdopts/cmdconfig.go
Outdated
| var upgraded bool | ||
|
|
||
| // Upgrade metrics/sources configuration if both are postgres | ||
| if hasMetricsPg && hasSourcesPg { |
There was a problem hiding this comment.
That's wrong. We must not depend on both options be the same of a kind. upgrade command must work with every option independently
There was a problem hiding this comment.
So basically , - if metrics is postgres, upgrade it; if sources is postgres, upgrade it separately. ? Ill look into it
Pull Request Test Coverage Report for Build 21245920934Details
💛 - Coveralls |
pashagolub
force-pushed
the
fix-sink-only-upgrade-1114
branch
from
816494c to
25531be
Compare
diiviikk5
force-pushed
the
fix-sink-only-upgrade-1114
branch
from
25531be to
51a709a
Compare
|
made them independent , @pashagolub , any other changes or feedback? |
internal/cmdopts/cmdconfig.go
Outdated
| hasMetricsPg := opts.IsPgConnStr(opts.Metrics.Metrics) | ||
| hasSourcesPg := opts.IsPgConnStr(opts.Sources.Sources) | ||
| hasSinks := len(opts.Sinks.Sinks) > 0 |
There was a problem hiding this comment.
Why only Postgres is here? What if later we want to upgrade YAML files from old format to a new one?
We should rely on checking if smth supports metrics.Migrator interface.
| if err = opts.ValidateConfig(); err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
That's a correct idea! Validating is needed for a normal work but for a command we shall check in-place.
|
also, please, do not overwrite my rebases ;-)
Please, make sure your branch is rebased against master |
When using yaml for sources/metrics, you can now run: pgwatch --sink=postgres://... config upgrade Previously this would fail asking for --sources and --metrics.
diiviikk5
force-pushed
the
fix-sink-only-upgrade-1114
branch
from
51a709a to
5d1c0f9
Compare
|
sorry couldnt update earlier @pashagolub i was ill ,but thanks for reviewing the code properly it helps me learn , i did the rebase against the latest master , Metrics, sources, and sinks are upgraded independently |
4 participants
Fixes #1114
The upgrade command now accepts
--sinkalone when sources/metrics use yaml files.Removed the validation that required both --sources and --metrics to be postgres URLs. Each config type is now upgraded independently if available.