sofie-core icon indicating copy to clipboard operation
sofie-core copied to clipboard

feat: move system storePath to an environment variable

Open Julusian opened this issue 1 year ago • 1 comments

  • 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

Julusian avatar Oct 17 '22 15:10 Julusian

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.

codecov-commenter avatar Oct 17 '22 15:10 codecov-commenter

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?

Julusian avatar Oct 28 '22 08:10 Julusian