fix: sink-only schema upgrade now works (#1114)#1138
fix: sink-only schema upgrade now works (#1114)#1138diiviikk5 wants to merge 1 commit intocybertec-postgresql:masterfrom
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 21474863984Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - 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? |
| 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.
internal/cmdopts/cmdconfig.go
Outdated
| 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 |
|
Test must pass |
diiviikk5
force-pushed
the
fix-sink-only-upgrade-1114
branch
2 times, most recently
from
4b07222 to
6c580f2
Compare
|
working on it , will inform / tag you once its done , will properly test it first before requesting workflow again |
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.