-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: global config sepc #1725
Merged
+153
β36
Merged
feat: global config sepc #1725
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
20f92ab
feat: initial work done to rely on new global config dir
JGAntunes 89c8c11
feat: add final functionality to the new global config spec
JGAntunes d392c67
chore: add comment to tests
JGAntunes aacac64
fix: typo in comment
JGAntunes 2e4554e
fix: don't fail tests if there's no config to backup
JGAntunes 8a7d8a9
fix: make tests resilient to missing config directories
JGAntunes c3a0671
fix: trolled by nodejs fs functions :facepalm:
JGAntunes 716bbdb
chore: don't delete legacy config and memoise globalConfig result
JGAntunes 8bc2d33
chore: dropping lodash.once as per #1728 and using memoize-one
JGAntunes e3cf50c
chore: dropping lodash.once as per #1728 and using memoize-one
JGAntunes 3e87c3d
fix: require memoizeOne :facepalm:
JGAntunes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
const Configstore = require('configstore') | ||
const memoizeOne = require('memoize-one') | ||
const { v4: uuidv4 } = require('uuid') | ||
|
||
const { readFileAsync } = require('../lib/fs') | ||
const { getPathInHome, getLegacyPathInHome } = require('../lib/settings') | ||
|
||
const globalConfigDefaults = { | ||
/* disable stats from being sent to Netlify */ | ||
telemetryDisabled: false, | ||
/* cliId */ | ||
cliId: uuidv4(), | ||
} | ||
|
||
const getGlobalConfig = async function () { | ||
const configPath = getPathInHome(['config.json']) | ||
// Legacy config file in home ~/.netlify/config.json | ||
const legacyPath = getLegacyPathInHome(['config.json']) | ||
let legacyConfig | ||
// Read legacy config if exists | ||
try { | ||
legacyConfig = JSON.parse(await readFileAsync(legacyPath)) | ||
} catch (_) {} | ||
// Use legacy config as default values | ||
const defaults = { ...globalConfigDefaults, ...legacyConfig } | ||
const configStore = new Configstore(null, defaults, { configPath }) | ||
|
||
return configStore | ||
} | ||
|
||
// Memoise config result so that we only load it once | ||
module.exports = memoizeOne(getGlobalConfig) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
const os = require('os') | ||
const path = require('path') | ||
|
||
const test = require('ava') | ||
|
||
const { | ||
rmdirRecursiveAsync, | ||
mkdirRecursiveAsync, | ||
readFileAsync, | ||
writeFileAsync, | ||
copyFileAsync, | ||
rmFileAsync, | ||
} = require('../lib/fs') | ||
const { getPathInHome, getLegacyPathInHome } = require('../lib/settings') | ||
|
||
const getGlobalConfig = require('./get-global-config.js') | ||
|
||
const configPath = getPathInHome(['config.json']) | ||
const legacyConfigPath = getLegacyPathInHome(['config.json']) | ||
const tmpConfigBackupPath = path.join(os.tmpdir(), `netlify-config-backup-${Date.now()}`) | ||
|
||
test.before('backup current user config if exists', async () => { | ||
try { | ||
await copyFileAsync(configPath, tmpConfigBackupPath) | ||
} catch (_) {} | ||
}) | ||
|
||
test.after.always('cleanup tmp directory and legacy config', async () => { | ||
try { | ||
// Restore user config if exists | ||
await mkdirRecursiveAsync(getPathInHome([])) | ||
await copyFileAsync(tmpConfigBackupPath, configPath) | ||
// Remove tmp backup if exists | ||
await rmFileAsync(tmpConfigBackupPath) | ||
} catch (_) {} | ||
// Remove legacy config path | ||
await rmdirRecursiveAsync(getLegacyPathInHome([])) | ||
}) | ||
|
||
test.beforeEach('recreate clean config directories', async () => { | ||
// Remove config dirs | ||
await rmdirRecursiveAsync(getPathInHome([])) | ||
await rmdirRecursiveAsync(getLegacyPathInHome([])) | ||
// Make config dirs | ||
await mkdirRecursiveAsync(getPathInHome([])) | ||
await mkdirRecursiveAsync(getLegacyPathInHome([])) | ||
}) | ||
|
||
// Not running tests in parallel as we're messing with the same config files | ||
|
||
test.serial('should use legacy config values as default if exists', async (t) => { | ||
const legacyConfig = { someOldKey: 'someOldValue', overrideMe: 'oldValue' } | ||
const newConfig = { overrideMe: 'newValue' } | ||
await writeFileAsync(legacyConfigPath, JSON.stringify(legacyConfig)) | ||
await writeFileAsync(configPath, JSON.stringify(newConfig)) | ||
|
||
const globalConfig = await getGlobalConfig() | ||
t.is(globalConfig.get('someOldKey'), legacyConfig.someOldKey) | ||
t.is(globalConfig.get('overrideMe'), newConfig.overrideMe) | ||
}) | ||
|
||
test.serial('should not throw if legacy config is invalid JSON', async (t) => { | ||
await writeFileAsync(legacyConfigPath, 'NotJson') | ||
await t.notThrowsAsync(getGlobalConfig) | ||
}) | ||
|
||
test.serial("should create config in netlify's config dir if none exists and store new values", async (t) => { | ||
// Remove config dirs | ||
await rmdirRecursiveAsync(getPathInHome([])) | ||
await rmdirRecursiveAsync(getLegacyPathInHome([])) | ||
|
||
const globalConfig = await getGlobalConfig() | ||
globalConfig.set('newProp', 'newValue') | ||
const configFile = JSON.parse(await readFileAsync(configPath)) | ||
t.deepEqual(globalConfig.all, configFile) | ||
}) |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[sand] Not sure there is big benefit for it, but would it make sense to use
lodash.once
to resolve the config only a single time.For example in
utils/command.js
the function will be invoked twice (once for the token and once for the whole config).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Good one, I actually tested it out locally and saw some specifc commands could actually trigger a call to
globalConfig
multiple times, I wanted to mention that but just forgot π€¦ I think some kind of memoisation likelodash.once
would definitely be a good thing here πThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestion:
memoize-one
is also an option. It does the same thing but the code looks much clearer. Lodash code always looks so verbose :/lodash.once()
:memoize-one
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @ehmicky! So I was actually looking at
lodash.once
and was considering alternatives, however seems like we already have lodash as adependency
- https://github.com/netlify/cli/blob/master/package.json#L132 - taking that into account would it be worth bringing another dependency? Might be something worth considering if we plan on decouplinglodash
from the project (or at least shift to depending only on the specific lodash modules we need).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Either works, your call π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we're keeping the "legacy" config path for now, would it be worth it creating an issue just to track this "tech debt"? CC @erezrokah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! π
About adding an issue for future dead code removal: this would be a good idea.
Quick question: was there any way to use
this.netlify.globalConfig
instead of callinggetGlobalConfig()
in the telemetry and hooks code? This would remove the need to memoize. Feel free to discard this comment if not relevant, I was just curious :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @ehmicky. I guess it would be possible and it would probably make a lot of sense for the
track/identify
telemetry functions to receive the required config via its parameters instead of requiring it. For the hooks however, I'm still digging through oclifs arch but seems like they don't have access to the base command instance - https://oclif.io/docs/hooks#lifecycle-events π. We also have this staticgetToken
function that depends on the global-config - https://github.com/netlify/cli/blob/master/src/utils/command.js#L313 - but seems like it's only being called on the deploy test - https://github.com/netlify/cli/blob/master/tests/command.deploy.test.js#L8 (?)Tbh I think it makes sense to avoid this memoisation π€ maybe eventually remove it. Just not 100% of what would be the impact of doing it right now, given the global-config seems to be tied to a couple of different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could move forward with
memoisation
and open another issue to refactor the code to usethis.netlify.globalConfig
.WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! π