sofie-core
sofie-core copied to clipboard
feat: move system storePath to an environment variable
- What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
- What is the current behavior? (You can also link to an open issue here)
The storePath used for snapshots is stored in the database.
- What is the new behavior (if this is a feature change)?
The storePath is really an environment level configuration. It is chosen at the time of first deploying the docker images (it has to be bound in as a volume), and so does not make sense to have as a dynamic configuration in the database.
It is now an environment variable instead. This lets it be defined by the person/tool running the docker commands, who have already made the decision about where the storepath will reside.
At startup, if this variable is not defined, it will cause sofie to crash with a message SOFIE_STORE_PATH must be defined to launch Sofie
For development, if the variable is not set, then a path .meteor/local/sofie-store
will be used instead. This simplifies setting up a development environment, and should keep impact to developers to a minimum.
- Other information:
Status
- [ ] Code documentation for the relevant parts in the code have been added/updated by the PR author
- [x] The functionality has been tested by the PR author
- [ ] The functionality has been tested by NRK
Codecov Report
Base: 68.19% // Head: 68.16% // Decreases project coverage by -0.02%
:warning:
Coverage data is based on head (
6c80a0b
) compared to base (a166d7b
). Patch coverage: 42.10% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@
## release47 #778 +/- ##
=============================================
- Coverage 68.19% 68.16% -0.03%
=============================================
Files 282 282
Lines 34714 34703 -11
Branches 4638 4623 -15
=============================================
- Hits 23673 23657 -16
- Misses 10602 10612 +10
+ Partials 439 434 -5
Impacted Files | Coverage Δ | |
---|---|---|
meteor/lib/collections/CoreSystem.ts | 82.82% <ø> (-0.66%) |
:arrow_down: |
meteor/server/migration/0_1_0.ts | 87.09% <ø> (-1.95%) |
:arrow_down: |
meteor/server/api/blueprints/api.ts | 82.03% <28.57%> (+2.64%) |
:arrow_up: |
meteor/server/migration/databaseMigration.ts | 59.29% <33.33%> (-0.44%) |
:arrow_down: |
meteor/server/api/snapshot.ts | 33.42% <42.85%> (+<0.01%) |
:arrow_up: |
meteor/server/coreSystem.ts | 62.20% <47.61%> (-2.20%) |
:arrow_down: |
meteor/lib/lib.ts | 69.83% <0.00%> (-0.56%) |
:arrow_down: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
We don't need a system status check, as it will explicitly crash at startup with a helpful error https://github.com/nrkno/sofie-core/blob/feat/system-store-path-as-env-var/meteor/server/coreSystem.ts#L54 https://github.com/nrkno/sofie-core/blob/feat/system-store-path-as-env-var/meteor/server/coreSystem.ts#L480-L482
This is consistent with how other environment variables are handled (MONGO_URL, ROOT_URL).
I went with a crash at startup as I think that is a better convention to follow for an environment variable that is definitely wrong/missing. This makes it clearer who is responsible for fixing it (startup crash saying something isnt setup = ops, systemstatus = sofie-admin). When deploying this as part of r48, is it reasonable to expect an ops person to read through the 15+ systemstatus warnings to check for one which is relevant to them, which they presumably arent expecting so dont know what to look for?