feat(Config): Introduce new config resolution #70

pgrzesik · 2021-01-23T17:26:28Z

Notifications tests had to be adjusted because they became nondeterministic - in some cases, the lastShown date for two notifications was the same and not the one that was expected was returned.

It introduces the following breaking changes:

Removed getGlobalConfig from API
removed support for all rc-discoverable files
update not always modify only global config (if local is present, it's the one that will get updated)

Closes: #69

codecov · 2021-01-23T18:46:00Z

Codecov Report

Merging #70 (668e7d9) into master (3f4cee4) will increase coverage by 8.33%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   80.83%   89.17%   +8.33%     
==========================================
  Files           8        8              
  Lines         167      194      +27     
==========================================
+ Hits          135      173      +38     
+ Misses         32       21      -11

Impacted Files	Coverage Δ
config.js	`92.63% <95.58%> (+19.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f4cee4...668e7d9. Read the comment docs.

pgrzesik · 2021-01-25T07:38:40Z

test/config.js

+    let globalConfigPath;
+
+    before(async () => {
+      await fs.promises.mkdir('local');


The reason for that construct being present in multiple tests is the fact that we're mocking both homedir and cwd to a temporary directory, which means that both local and global config has the same location in these scenarios. I'd love to hear feedback on how to potentially improve that in the scope of these tests

I think you've taken very good approach, I cannot think of anything better 👍

medikoo

Looks really good! It's great to see so extensive testing (after all it's a very sensitive functionality)

I have just few comments. Let me know

medikoo · 2021-01-25T12:44:23Z

config.js

-  config.meta.updated_at = Math.round(Date.now() / 1000);
-  // write to .serverlessrc file
-  return storeConfig(config);
+  return storeConfig(config, configPath, { updated: true });


I think in all cases we should return same value as getConfig would return (in light of that I would strip returning any value from storeConfig)

I think we can skip returning anything from storeConfig 👍 The question here is - should we return anything from storeConfig then? I was thinking about true/false for indicating if save was successful or not or maybe we should just throw any potential errors?

medikoo · 2021-01-25T12:46:56Z

config.js

  try {
-    writeFileAtomic.sync(serverlessrcPath, JSON.stringify(config, null, 2));
+    if (options.updated) {


Why do we need that option, can't we store it unconditionally?

I wanted to keep previous behavior when on default global config creation updated_at was not populated but I'm fine with updating it unconditionally. What do you think?

I would update it unconditionally. I think there's no valid reason to be conditional here.

updated 👍

medikoo · 2021-01-25T12:49:26Z

config.js

+  if (!config.userId) {
+    return null;
+  }
+  const user = get(['users', config.userId, 'dashboard'], config);


As we already resolved config, probably more optimal would be to resort o _.get(config, path) here (?)

definitely, I totally missed that when moving functionality from platform-sdk

config.js

medikoo · 2021-01-25T12:57:08Z

test/config.js

+
+  it('should have CONFIG_FILE_NAME', () => {
+    const configFilename = config.CONFIG_FILE_NAME;
+    expect(configFilename).to.exist; // eslint-disable-line


I think we don't need this eslint disable comment (I know it's like that in master, but I guess we can clean this now)

medikoo · 2021-01-25T12:59:41Z

test/process-backend-notification-request.js

+  let clock;
+
+  before(() => {
+    clock = sinon.useFakeTimers(Date.now());


Why those tests needed to be updated?

I've tried to clarify that on lines 27-28. Essentially, it turned out that after refactoring, new config resolution is faster than rc and this test started to fail from time to time. After inspection, it was caused by the fact that some of the notifications were emitted almost at the exact same time, which caused them to have the same lastShown value. That in turn resulted in the functionality selecting the next notification to be emitted to be resolved incorrectly (I've observed situations where CODE0A was selected instead of CODE0C because they had the same lastShown date.

I see, good point, but in such case I'd rather introduce await wait(1) between calls (with a comment that it ensures distinct ms timestamps)

At least (but maybe it's just my bias) interfering with JS clock feels quite invasive to me. I'd rely on it, as a last resort (where e.g. we need simulate longer periods of times, or precisely reproduce some time sequences)

I think wait(1) here makes sense and is definitely enough 👍 I might be heavily biased myself due to my background in Python, where mocking time is much more common practice. I've changed to the suggested approach 👍

medikoo · 2021-01-25T13:03:22Z

test/config.js

+  before(() => {
+    clock = sinon.useFakeTimers(now);
+  });
+
+  after(() => {
+    clock.restore();
+  });


I take, it's to test, the updated_at, but isn't it overkill? Can't we simply do setTimeout(.., 10) and confirm that updated_at was updated (?) If it appears as needed, maybe it'll be nicer to mock timers only for tests where we test that functionality and not whole test file (?)

I agree, it might be a bit of an overkill here. I'll adjust it 👍

medikoo · 2021-01-25T13:04:07Z

test/config.js

+    let globalConfigPath;
+
+    before(async () => {
+      await fs.promises.mkdir('local');


I think you've taken very good approach, I cannot think of anything better 👍

medikoo

Looks great @pgrzesik ! I've posted just a few minor suggestions

medikoo · 2021-01-26T08:20:39Z

config.js

+    config.meta = config.meta || {};
+    config.meta.updated_at = Math.round(Date.now() / 1000);


I would remove those two from try. Technically this clause is dedicated to catch write error and nothing else.

To be fully bulletproof also config stringification is best if outside of try/catch clause (if it crashes it feels as programmer error on our side, which I would expose unconditionally)

medikoo · 2021-01-26T10:46:30Z

config.js

-  config.meta.updated_at = Math.round(Date.now() / 1000);
-  // write to .serverlessrc file
-  return storeConfig(config);
+  return storeConfig(config, configPath);


As now storeConfig doesn't return anything, I think we need to return result of getConfig here

medikoo · 2021-01-26T10:46:40Z

config.js

  if (key && typeof key === 'string') {
    config = _.omit(config, [key]);
  } else if (key && Array.isArray(key)) {
    config = _.omit(config, key);
  }
-  // write to .serverlessrc file
-  return storeConfig(config);
+  return storeConfig(config, configPath);


BREAKING CHANGE: Removed `getGlobalConfig`, removed support for all `rc`-discoverable files, update not always modify only global config

medikoo

Great job! 👍

pgrzesik force-pushed the feature-adjust-config-util branch from 7d94871 to d1be3c6 Compare January 23, 2021 17:28

pgrzesik added the enhancement New feature or request label Jan 23, 2021

pgrzesik force-pushed the feature-adjust-config-util branch 2 times, most recently from bc3b25f to c22141a Compare January 23, 2021 18:44

pgrzesik self-assigned this Jan 24, 2021

pgrzesik requested a review from medikoo January 25, 2021 07:36

pgrzesik commented Jan 25, 2021

View reviewed changes

medikoo suggested changes Jan 25, 2021

View reviewed changes

pgrzesik force-pushed the feature-adjust-config-util branch 3 times, most recently from c8f2be1 to 0f86d99 Compare January 25, 2021 15:04

pgrzesik requested a review from medikoo January 25, 2021 16:27

medikoo suggested changes Jan 26, 2021

View reviewed changes

feat: Introduce new config resolution

668e7d9

BREAKING CHANGE: Removed `getGlobalConfig`, removed support for all `rc`-discoverable files, update not always modify only global config

pgrzesik force-pushed the feature-adjust-config-util branch from 0f86d99 to 668e7d9 Compare January 26, 2021 11:04

pgrzesik requested a review from medikoo January 26, 2021 11:14

medikoo approved these changes Jan 26, 2021

View reviewed changes

pgrzesik merged commit 3bff1b4 into master Jan 26, 2021

pgrzesik deleted the feature-adjust-config-util branch January 26, 2021 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Config): Introduce new config resolution #70

feat(Config): Introduce new config resolution #70

pgrzesik commented Jan 23, 2021

codecov bot commented Jan 23, 2021 •

edited

pgrzesik Jan 25, 2021 •

edited

medikoo Jan 25, 2021

medikoo left a comment

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021 •

edited

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

medikoo Jan 25, 2021

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

medikoo Jan 25, 2021

medikoo left a comment

medikoo Jan 26, 2021

medikoo Jan 26, 2021

medikoo Jan 26, 2021

medikoo left a comment

		config.meta = config.meta \|\| {};
		config.meta.updated_at = Math.round(Date.now() / 1000);

feat(Config): Introduce new config resolution #70

feat(Config): Introduce new config resolution #70

Conversation

pgrzesik commented Jan 23, 2021

codecov bot commented Jan 23, 2021 • edited

Codecov Report

pgrzesik Jan 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik Jan 25, 2021 • edited

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

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

Choose a reason for hiding this comment

medikoo 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

medikoo left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2021 •

edited

pgrzesik Jan 25, 2021 •

edited

pgrzesik Jan 25, 2021 •

edited