Optional YAML configuration for Brig by elverkilde · Pull Request #90 · wireapp/wire-server

elverkilde · 2017-10-12T08:54:53Z

Adds optional YAML parsing for loading the configuration, the old commandline options are still supported. Only brig and brig-integration is implemented, brig-schema and brig-index needs to be done.

jschaul · 2017-10-13T11:19:04Z

services/brig/brig.cabal

      , types-common
      , uri-bytestring

-test-suite brig-integration


what's the motivation of changing the type here?

test-suite implies running them as part of the build, which in the current code is worked around by requiring a certain environment variable. In reality, it is actually some sort of mix between a test and an executable.

This way, we should be able to build things (as before) without the need of a hack in the test executable.

Okay. Please then also take out this hacky line in the Makefile: https://github.com/wireapp/wire-server/blob/develop/services/brig/Makefile#L31

jschaul · 2017-10-13T11:24:41Z

services/brig/src/Brig/Options.hs

+    , port :: Word16
+    } deriving (Show, Generic)
+
+instance FromJSON Endpoint where


remove unnecessary trailing where keywords?

yes, I just found out through hindent that they aren't required

jschaul · 2017-10-13T11:43:10Z

services/brig/src/Brig/Options.hs

    , setWhitelist          :: !(Maybe Whitelist)
    , setUserMaxConnections :: !Int64
-    , setCookieDomain       :: !ByteString
+    , setCookieDomain       :: Text


Text -> !Text to ensure those are defined on startup?

jschaul · 2017-10-13T11:59:44Z

services/brig/src/Brig/Whitelist.hs

-    , whitelistUser :: !ByteString
-    , whitelistPass :: !ByteString
-    } deriving Show
+    { whitelistReq  :: !Text


rename as whitelistUrl?

jschaul · 2017-10-13T12:01:00Z

services/brig/src/Brig/ZAuth.hs

+instance FromJSON UserTokenTimeout where
+instance FromJSON SessionTokenTimeout where
+instance FromJSON AccessTokenTimeout where
+instance FromJSON ProviderTokenTimeout where


remove where?

jschaul · 2017-10-13T12:04:47Z

services/brig/src/Main.hs

+                 <> showDefault
+                 <> value defaultPath
+    desc = header "Brig - User Service" <> fullDesc
+


instead of putting this into Main, perhaps move to one of the shared libraries? That would avoid duplication regarding other services & avoid having to add more dependencies to every build-deps in cabal files.

I agree, is there an existing lib where this would fit?

types-common would work I suppose.

jschaul · 2017-10-13T12:10:38Z

services/brig/test/integration/API/Provider.hs

+
+instance FromJSON Config where
+
+tests :: Either ParseException Config -> Manager -> DB.ClientState -> Brig -> Cannon -> Galley -> IO TestTree


can we avoid passing a Either ParseException Config into every test? That failure arising from a missing configuration entry or missing ENV variable could be checked before starting any test. Would be better to handle this in Main.

yes, that would have been preferable, but I don't see how this can be resolved in main since the environment vars are pulled directly in the tests.

Would Maybe Config help?

jschaul · 2017-10-16T10:24:22Z

services/brig/test/integration/Main.hs

-                . Cql.addContact h
+main = withOpenSSL $ do
+  iConf <- decodeConfigFile "/etc/wire/brig/integration.yaml"
+  bConf <- decodeConfigFile "/etc/wire/brig/brig.yaml"


Can we make these locations configurable, please?

jschaul · 2017-10-16T10:25:47Z

services/brig/src/Main.hs

+                 <> showDefault
+                 <> value defaultPath
+    desc = header "Brig - User Service" <> fullDesc
+


types-common would work I suppose.

tiago-loureiro · 2017-10-21T20:01:11Z

libs/types-common/src/Util/Options.hs

+import Options.Applicative
+import System.Directory
+import System.Environment (getArgs)
+import System.IO (hPutStrLn, stderr)


Can we instead use our logger as, for instance, here which logs messages in a format that we can already parse?

This is intended to be used before or without configuring the logging framework. This is basically a temporary workaround, until we decide for either options or config files. When we have one way to configure the app, we will simply terminate it with an error as before this PR.

I would rather remove "logging" from this file, as it is not really an error anyway.

OK, let's leave it as-is then 👍

jschaul · 2017-10-23T12:42:59Z

services/brig/Makefile

 	docker build -t $(executable) \
-	-f ../../build/alpine/Dockerfile \
+	-f Dockerfile \
 	--build-arg service=$(NAME) \


--build-arg service=$(NAME) \ can be removed.

elverkilde · 2017-10-23T14:28:24Z

@tiago-loureiro @jschaul, I did a rebase on develop and squashed.

elverkilde requested review from jschaul and tiago-loureiro October 12, 2017 08:54

jschaul reviewed Oct 13, 2017

View reviewed changes

jschaul reviewed Oct 16, 2017

View reviewed changes

elverkilde force-pushed the feature/brig-yaml branch from 4ca4fb9 to 2d62160 Compare October 20, 2017 11:03

tiago-loureiro reviewed Oct 21, 2017

View reviewed changes

jschaul approved these changes Oct 23, 2017

View reviewed changes

elverkilde force-pushed the feature/brig-yaml branch from dd1ce87 to 99c1934 Compare October 23, 2017 14:27

Optional YAML configuration for Brig

2df5282

elverkilde force-pushed the feature/brig-yaml branch from 99c1934 to 2df5282 Compare October 23, 2017 14:35

elverkilde merged commit ef01acb into develop Oct 23, 2017

elverkilde deleted the feature/brig-yaml branch October 23, 2017 14:58

mdimjasevic mentioned this pull request Oct 30, 2023

Fix galley DB migrations #3680

Merged

2 tasks


		instance FromJSON Config where

		tests :: Either ParseException Config -> Manager -> DB.ClientState -> Brig -> Cannon -> Galley -> IO TestTree

Comments

Conversation

elverkilde commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elverkilde commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elverkilde commented Oct 23, 2017 •

edited

Loading