(#1) BASH-20 Add base and override config #586

csduarte · 2017-09-08T16:22:53Z

Description
This PR moves configuration values to a base.json file (src/common/config/base.json) and allow to override those config values on file override.json (src/common/config/override.json). That way the override values can be stored on a separate repository when building the clientes.

read and understood our Contributing Guidelines
completed Mattermost Contributor Agreement
executed npm run lint:js for proper code formatting

Test Cases
Add override values like this:

// Example override.js
{
  "1": {
    "showTrayIcon": true,
    "trayIconTheme": "light"
  }
}

yuya-oc · 2017-09-09T02:55:28Z

Thanks for a lot of PRs!

This mechanism doesn't require Electron, so would you write tests in test/specs/settings_test.js? At least, upgrading from v0 to v1 is breaking. You can use describe.only() or it.only() to execute only your tests.

I thinks this PR forces only default settings. So users can change all settings. Is this correct?

yuya-oc · 2017-09-09T02:57:16Z

Just curious, why does override.json require version number? I feel it's enough there is one config in the file.

yuya-oc

Please write tests for overriding.

yuya-oc · 2017-09-09T03:01:06Z

src/common/settings.js

+function deepMergeArray(dest) {
+  return dest;
+}
+
 function loadDefault(version, spellCheckerLocale) {


Please test how loadDefault() works. Possibly require('override.json') doesn't match for writing tests because its static file.

@yuya-oc do you want an automated test?

We followed the exact same style as the mobile app in terms of config and override. Possibly would like to match that to start with and then diverge?

@csduarte If possible. At least, I want that existing tests (test/specs/settings_test.js) is passed.
Unfortunately I'm not sure about mobile app source code. Would you tell me the part?

yuya-oc · 2017-09-09T03:53:46Z

@jasonblais This would be one of solution for #483, but admins have to build apps from source on their own. Of course code-signing would be gone when they don't. Is this beneficial?

yuya-oc · 2017-09-14T11:27:26Z

Please let me confirm. What is the purpose of this feature? At first, I thought that this PR let users use pre-configured preferences.
However users can change their preferences. And in other PRs, this feature is also used to change "non user-preferences" behavior like ./configure.sh of C/C++ projects. I feel this will be confusing later, so this should be separated to two modules.

To change default preferences, it's enough that the maintainer edits base.json.

csduarte · 2017-09-14T16:47:35Z

@yuya-oc We were trying to make the smallest possible changes, because we know that getting bigger changes back is quite hard. So we used the only config there was to allow what we think of 'Feature Flags'.

Being able to flip something on or off, sometimes dynamically. -- If you could imagine, in the future, we could be developing a brand new feature in the master branch, but have it disabled because of a flag in this config, and that could be flipped. So it certainly crosses the bounds between system config and user config, but eventually we decided a division doesn't make a lot of sense. Just some configs are visible to the user and some are not, and maybe an admin could have access to all of them to flip on and off.

jasonblais · 2017-09-15T18:16:43Z

src/package.json

@@ -12,6 +12,7 @@
    "auto-launch": "^5.0.1",
    "bootstrap": "^3.3.7",
    "create-react-class": "^15.5.3",
+    "deepmerge": "^1.5.1",


Is assume this is from https://github.com/KyleAMathews/deepmerge?

correct, also seen here https://www.npmjs.com/package/deepmerge

jasonblais · 2017-09-15T18:19:08Z

@csduarte In terms of the use case, when developing a new feature in the master branch, we'd have it turned on by default, but have that setting disabled via config/override.json until the feature is finished? Is that correct?

csduarte · 2017-09-15T20:12:35Z

@jasonblais The exact use case is quote flexible. In products that I know better, there is actually another service that will give the config option to the user on boot and show/hide certain features. Basically, An easy way to to A/B testing and test features, all while maintaining continuous integration and deployment.

jasonblais · 2017-09-18T13:40:22Z

Thanks! The context is very helpful.

EDIT: Chatted further with @yuya-oc and we're open to taking the PR, but there might be a few small adjustments that we'd propose. I'll make another post here by end of the day.

jasonblais · 2017-09-18T18:24:20Z

Hey @dmeza, @csduarte

Not urgent, but wondering for your thoughts on the following:

1 - Having both default and 1 lists in the /config/base.json file might be confusing to admins/developers building the apps from the source. It's not immediately obvious why there are two sets of default settings present and what their purpose is.

Would it make sense to split this file into base.json (containing "default") and pastDefault.json (containing "1", and other versions in the future)?

2 - Wondering your thoughts about keeping the core PR the same, but splitting the base.json file into defaultPreferences.json (user config) and buildConfig.json (system config)?

^Open for feedback, as we want to keep further changes from you minimal.

dmeza · 2017-09-27T21:02:23Z

Hey @jason, @yuya-oc,
Sorry for the delay to reply. We've been focused on mobile and platform.
Our proposed changes/solution is based on the code that's already there, we're open to getting to the same result with different changes. I have a question as if default and 1 are really needed or there could be a different solution?
Our goal is having configuration values that can enable/disable features and for white-labeling, so both of the proposed changes work. @csduarte explains it here: #586 (comment)

dmeza · 2017-09-29T04:35:40Z

@yuya-oc: settings tests are passing now. Fixed the upgrade test and added a test for loadDefault.

yuya-oc · 2017-09-29T11:16:13Z

test/modules/environment.js

@@ -1,4 +1,5 @@
 'use strict';
+process.env.TEST = 'test';


Was this necessary for you?

@yuya-oc this the packages context for build and test seems to be different and needed a different deepmerge require:
let deepmerge = require('deepmerge').default; if (process.env.TEST) { deepmerge = require('deepmerge'); // eslint-disable-line }
https://github.com/mattermost/desktop/pull/586/files#diff-7cdc7778ed182028c72b7c53ac2af20bR4

@dmeza Got it. Assuming that, this sentence is not needed. When src/common/settings.js requires deepmerge, it correctly uses src/node_modules/deepmerge. Probably you met the error of that test/specs/settings_test.js requires deepmerge at outside of src/.

yuya-oc · 2017-09-29T11:23:07Z

test/specs/settings_test.js

+    const baseConfig = require('../../src/common/config/base.json');
+    const overrideConfig = require('../../src/common/config/override.json');
+    const expectedDefaults = deepmerge(
+      baseConfig[1], overrideConfig[1] || {}, {clone: true, arrayMerge: settings.deepMergeArray}


This part is actually same to loadDefault() implementation. So this test always passes. Is this intended?

@yuya-oc I think that it will provide the context to be able to test specific base config and override changes. What other test do you think we should add here?

@dmeza I had imagined the test simply comparing to "expected" default config. So in this case, something like expectedDefaults = { teams: [], showTrayIcon: false, ... } instead of calling deepmerge() because a plain object is obvious as an expected value.
However, surely this might be enough for now. I can accept.

yuya-oc · 2017-09-29T11:25:29Z

package.json

@@ -60,5 +61,6 @@
    "webpack": "^2.5.1",
    "webpack-dev-server": "^2.4.5",
    "webpack-merge": "^4.1.0"
-  }
+  },
+  "dependencies": {}


This seems accidentally changed.

dmeza · 2017-09-29T16:46:22Z

Hey @jason, @yuya-oc, one way to simplify is to have a single version on the override.json file.
Not this:
// Example override.js { "1": { "showTrayIcon": true, "trayIconTheme": "light" } }
But this:
// Example override.js { "showTrayIcon": true, "trayIconTheme": "light" }

yuya-oc

Sorry for late reply and many requests. Would you confirm again?

yuya-oc · 2017-10-02T14:40:47Z

test/modules/environment.js

@@ -1,4 +1,5 @@
 'use strict';
+process.env.TEST = 'test';


@dmeza Got it. Assuming that, this sentence is not needed. When src/common/settings.js requires deepmerge, it correctly uses src/node_modules/deepmerge. Probably you met the error of that test/specs/settings_test.js requires deepmerge at outside of src/.

yuya-oc · 2017-10-02T14:53:45Z

test/specs/settings_test.js

+    const baseConfig = require('../../src/common/config/base.json');
+    const overrideConfig = require('../../src/common/config/override.json');
+    const expectedDefaults = deepmerge(
+      baseConfig[1], overrideConfig[1] || {}, {clone: true, arrayMerge: settings.deepMergeArray}


@dmeza I had imagined the test simply comparing to "expected" default config. So in this case, something like expectedDefaults = { teams: [], showTrayIcon: false, ... } instead of calling deepmerge() because a plain object is obvious as an expected value.
However, surely this might be enough for now. I can accept.

yuya-oc · 2017-10-05T15:39:22Z

src/common/settings.js

@@ -1,35 +1,37 @@
 'use strict';

 const fs = require('fs');
+let deepmerge = require('deepmerge').default;


As mentioned at #586 (comment), const deepmerge = require('deepmerge') is enough.

@yuya-oc this form const deepmerge = require('deepmerge') failed when building. Did you try it and worked for you for both cases building and running tests?

@dmeza Thanks for feedback. Somehow deepmerge becomes object from function in webpack. So error occurs at run time.
I'm not sure whether it's bug or spec of webpack though, let's keep this for now.

dmeza · 2017-10-09T15:46:14Z

@yuya-oc from your comments I gather that this PR is ready, right?

yuya-oc · 2017-10-09T23:18:05Z

@dmeza Ah, yeah. I had not been sure whether you are preparing more updates about simplified override.json or splitting to two config files.
If there are no more updates here, I think it’s ready to merge.

cc: @jasonblais

jasonblais · 2017-10-11T12:26:53Z

Thanks all! Let's go ahead and merge the PR.

yuya-oc · 2017-10-11T12:45:15Z

@csduarte @dmeza Huge thanks for many iteration!

yuya-oc suggested changes Sep 9, 2017

View reviewed changes

yuya-oc added the All Platforms null label Sep 9, 2017

jasonblais added this to the v3.8.0 milestone Sep 15, 2017

jasonblais reviewed Sep 15, 2017

View reviewed changes

jasonblais mentioned this pull request Sep 15, 2017

BASH-11 Open help link from config #593

Merged

3 tasks

jasonblais mentioned this pull request Sep 18, 2017

Update NOTICE.txt #603

Merged

2 tasks

Add base and override config

0efe676

dmeza force-pushed the bash-20 branch from b5d4b2c to 9d129b5 Compare September 29, 2017 04:26

yuya-oc reviewed Sep 29, 2017

View reviewed changes

Changes to have settings tests pass. test/specs/settings_test.js

76ced43

dmeza force-pushed the bash-20 branch from 9d129b5 to 76ced43 Compare September 29, 2017 16:14

yuya-oc reviewed Oct 2, 2017

View reviewed changes

yuya-oc reviewed Oct 5, 2017

View reviewed changes

yuya-oc approved these changes Oct 11, 2017

View reviewed changes

yuya-oc merged commit 38fcdf6 into mattermost:master Oct 11, 2017

jasonblais mentioned this pull request Oct 11, 2017

Simplify override.json and split config.json into two files #618

Closed

3 tasks

yuya-oc mentioned this pull request Oct 16, 2017

Fix an issue where the combination of deepmerge and webpack breaks tests #625

Merged

3 tasks

jasonblais mentioned this pull request Oct 17, 2017

Allow admins to pre-configure desktop apps #483

Closed

jasonblais mentioned this pull request Oct 25, 2017

Simplify and split config.json into defaultPreferences and buildConfig #633

Merged

3 tasks

(#1) BASH-20 Add base and override config #586

(#1) BASH-20 Add base and override config #586

Conversation

csduarte commented Sep 8, 2017 • edited Loading

yuya-oc commented Sep 9, 2017

yuya-oc commented Sep 9, 2017

yuya-oc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuya-oc Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

yuya-oc commented Sep 9, 2017

yuya-oc commented Sep 14, 2017 • edited Loading

csduarte commented Sep 14, 2017

jasonblais Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonblais commented Sep 15, 2017

csduarte commented Sep 15, 2017

jasonblais commented Sep 18, 2017 • edited Loading

jasonblais commented Sep 18, 2017

dmeza commented Sep 27, 2017 • edited Loading

dmeza commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmeza commented Sep 29, 2017

yuya-oc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmeza commented Oct 9, 2017

yuya-oc commented Oct 9, 2017

jasonblais commented Oct 11, 2017

yuya-oc commented Oct 11, 2017

csduarte commented Sep 8, 2017 •

edited

Loading

yuya-oc Sep 18, 2017 •

edited

Loading

yuya-oc commented Sep 14, 2017 •

edited

Loading

jasonblais Sep 15, 2017 •

edited

Loading

jasonblais commented Sep 18, 2017 •

edited

Loading

dmeza commented Sep 27, 2017 •

edited

Loading