From 4394f0bb8f203b32d42321d7c3329603614cdf9e Mon Sep 17 00:00:00 2001 From: creotutar Date: Wed, 28 Oct 2020 15:28:03 -0400 Subject: [PATCH 1/2] postupgrade: Fix script for submodules, merge correctly, keep .git We fix the postupgrade script by doing two things: (1) Only remove files in the theme folder if it is not a submodule. The default for the jambo init command is to have theme folders added as submodules. Before we were removing the .git folder which made it so that we could not see the history of the theme folder. We can now see the full theme git history after an upgrade. (2) Merge the correct global_config files. Before, we were merging the theme global_config.json pre-upgrade and post-upgrade. That is, we did not touch the user's global_config.json. This is not the desired behavior, and instead, we would like to see new fields from any new theme global_config.json file in the user-defined config/global_config.json. So we change the files being merged. Note: This means that on postupgrade, we update the user's config/global_config.json with any new fields. We do not delete any fields. Note: This does not touch legacy upgrades. Add tests for postupgrade merge JSON to make sure we keep certain assumptions about this global_config merge. First, it should always give precedence to the original config. Second, it should not delete any custom fields added to the global_config. Third, it should be completely additive and add new fields if they exist in any of the configuration parameters. J=SLAP-578 TEST=manual npm test Test on a completely new `jambo init` site with the HH theme. Try `jambo upgrade` and make sure there are no errors. The error we don't want to see is below. ``` fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'. Error: fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'. at GitExecutorChain.onFatalException (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:60:87) at GitExecutorChain. (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:51:28) at Generator.throw () at rejected (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:6:65) at processTicksAndRejections (internal/process/task_queues.js:97:5) Error occurred on node version: v12.16.1 ``` Used below command to clear the folder ``` rm -rf ./* && rm -rf .git* && jambo init && jambo import --theme answers-hitchhiker-theme && npm install && git add -A && git commit -m "init" ``` --- postupgrade/PostUpgradeHandler.js | 20 ++++++++++++++------ postupgrade/utils.js | 14 ++++++++++++++ tests/postupgrade/config1.json | 5 +++++ tests/postupgrade/config2.json | 5 +++++ tests/postupgrade/config3.json | 8 ++++++++ tests/postupgrade/utils.js | 19 +++++++++++++++++++ 6 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 tests/postupgrade/config1.json create mode 100644 tests/postupgrade/config2.json create mode 100644 tests/postupgrade/config3.json create mode 100644 tests/postupgrade/utils.js diff --git a/postupgrade/PostUpgradeHandler.js b/postupgrade/PostUpgradeHandler.js index a2a5f4bc7..aa8c4a1f9 100644 --- a/postupgrade/PostUpgradeHandler.js +++ b/postupgrade/PostUpgradeHandler.js @@ -1,7 +1,7 @@ const fs = require('fs'); const fsExtra = require('fs-extra'); const path = require('path'); -const { mergeJson } = require('./utils'); +const { mergeJson, isGitSubmodule } = require('./utils'); const simpleGit = require('simple-git/promise')(); /** @@ -15,13 +15,19 @@ class PostUpgradeHandler { } async handlePostUpgrade() { - this.removeFromTheme('.git', '.gitignore', 'tests'); + if (!isGitSubmodule(this.themeDir)) { + this.removeFromTheme('.git', '.gitignore', 'tests'); + } + + const userDefinedGlobalConfigPath = + path.relative(process.cwd(), path.join(this.configDir, this.globalConfigFile)); const themeGlobalConfigPath = path.relative(process.cwd(), path.join(this.themeDir, this.globalConfigFile)); if (fsExtra.pathExistsSync(themeGlobalConfigPath)) { - const mergedGlobalConfig = await this.mergeThemeGlobalConfig(themeGlobalConfigPath); - fs.writeFileSync(themeGlobalConfigPath, mergedGlobalConfig); + const mergedGlobalConfig = await this.mergeThemeGlobalConfig(userDefinedGlobalConfigPath, themeGlobalConfigPath); + fs.writeFileSync(userDefinedGlobalConfigPath, mergedGlobalConfig); } + this.copyStaticFilesToTopLevel( 'package.json', 'Gruntfile.js', 'webpack-config.js', 'package-lock.json'); } @@ -37,10 +43,12 @@ class PostUpgradeHandler { /** * Merges the theme's global_config with the user's current global_config. + * @param {string} userDefinedGlobalConfigPath The path to the user's current global config + * @param {string} themeGlobalConfigPath The path to the global config in the theme */ - async mergeThemeGlobalConfig(themeGlobalConfigPath) { + async mergeThemeGlobalConfig(userDefinedGlobalConfigPath, themeGlobalConfigPath) { const updatedCommentJson = fs.readFileSync(themeGlobalConfigPath, 'utf-8'); - const originalCommentJson = await simpleGit.show([`HEAD:${themeGlobalConfigPath}`]); + const originalCommentJson = fs.readFileSync(userDefinedGlobalConfigPath, 'utf-8'); return mergeJson(updatedCommentJson, originalCommentJson); } diff --git a/postupgrade/utils.js b/postupgrade/utils.js index 1c947c0b6..ba87ffb63 100644 --- a/postupgrade/utils.js +++ b/postupgrade/utils.js @@ -1,6 +1,7 @@ const fs = require('fs'); const { parse, assign, stringify } = require('comment-json'); const path = require('path'); +const simpleGit = require('simple-git/promise')(); /** * Merges two comment-json files, with the original having higher priority. @@ -52,3 +53,16 @@ function removeEmptyDirectoriesRecursively(dirpath) { } } exports.removeEmptyDirectoriesRecursively = removeEmptyDirectoriesRecursively; + +/** + * Returns whether the given file path is registered as a git submodule. + * @param {string} submodulePath + * @returns {boolean} + */ +async function isGitSubmodule(submodulePath) { + const submodulePaths = await simpleGit.subModule(['foreach', '--quiet', 'echo $sm_path']); + return !!submodulePaths + .split('\n') + .find(p => p === submodulePath) +} +exports.isGitSubmodule = isGitSubmodule; diff --git a/tests/postupgrade/config1.json b/tests/postupgrade/config1.json new file mode 100644 index 000000000..a6c16c700 --- /dev/null +++ b/tests/postupgrade/config1.json @@ -0,0 +1,5 @@ +{ + // Comment1 + "one": "hi", // Inline comment + "onetwo": "hello" +} diff --git a/tests/postupgrade/config2.json b/tests/postupgrade/config2.json new file mode 100644 index 000000000..d2f0ed658 --- /dev/null +++ b/tests/postupgrade/config2.json @@ -0,0 +1,5 @@ +{ + "two": "bye", + "onetwo": "goodbye" + // Comment2 +} diff --git a/tests/postupgrade/config3.json b/tests/postupgrade/config3.json new file mode 100644 index 000000000..c1c442951 --- /dev/null +++ b/tests/postupgrade/config3.json @@ -0,0 +1,8 @@ +{ + // Comment1 + "one": "hi", // Inline comment + "onetwo": "goodbye" + // Comment2 + , + "two": "bye" +} diff --git a/tests/postupgrade/utils.js b/tests/postupgrade/utils.js new file mode 100644 index 000000000..0ecb02cb4 --- /dev/null +++ b/tests/postupgrade/utils.js @@ -0,0 +1,19 @@ +const { + mergeJson +} = require('postupgrade/utils.js'); +const fs = require('fs'); + +describe('postupgrade utils', () => { + describe('mergeJson', () => { + it('puts together two comment JSON strings', () => { + const config1 = fs.readFileSync('./tests/postupgrade/config1.json', 'utf-8'); + const config2 = fs.readFileSync('./tests/postupgrade/config2.json', 'utf-8'); + const expected = fs.readFileSync('./tests/postupgrade/config3.json', 'utf-8').trim(); + + // if this passes, then it is correctly not deleting any fields or comments + // and it is giving precedence to the second parameter config + const mergedConfig = mergeJson(config1, config2); + expect(mergedConfig).toEqual(expected); + }); + }); +}); From 06a4ad2cfd69635dc23593ed815b417978680af4 Mon Sep 17 00:00:00 2001 From: creotutar Date: Wed, 28 Oct 2020 23:25:43 -0400 Subject: [PATCH 2/2] PR fixes: syntax, remove trim, var name change --- postupgrade/PostUpgradeHandler.js | 13 +++++++------ tests/postupgrade/config3.json | 2 +- tests/postupgrade/utils.js | 6 ++---- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/postupgrade/PostUpgradeHandler.js b/postupgrade/PostUpgradeHandler.js index aa8c4a1f9..6028619b0 100644 --- a/postupgrade/PostUpgradeHandler.js +++ b/postupgrade/PostUpgradeHandler.js @@ -19,13 +19,13 @@ class PostUpgradeHandler { this.removeFromTheme('.git', '.gitignore', 'tests'); } - const userDefinedGlobalConfigPath = + const userGlobalConfigPath = path.relative(process.cwd(), path.join(this.configDir, this.globalConfigFile)); const themeGlobalConfigPath = path.relative(process.cwd(), path.join(this.themeDir, this.globalConfigFile)); if (fsExtra.pathExistsSync(themeGlobalConfigPath)) { - const mergedGlobalConfig = await this.mergeThemeGlobalConfig(userDefinedGlobalConfigPath, themeGlobalConfigPath); - fs.writeFileSync(userDefinedGlobalConfigPath, mergedGlobalConfig); + const mergedGlobalConfig = await this.mergeThemeGlobalConfig(userGlobalConfigPath, themeGlobalConfigPath); + fs.writeFileSync(userGlobalConfigPath, mergedGlobalConfig); } this.copyStaticFilesToTopLevel( @@ -43,12 +43,13 @@ class PostUpgradeHandler { /** * Merges the theme's global_config with the user's current global_config. - * @param {string} userDefinedGlobalConfigPath The path to the user's current global config + * @param {string} userGlobalConfigPath The path to the user's current global config * @param {string} themeGlobalConfigPath The path to the global config in the theme + * @return {string} The comment-json merged global config */ - async mergeThemeGlobalConfig(userDefinedGlobalConfigPath, themeGlobalConfigPath) { + async mergeThemeGlobalConfig(userGlobalConfigPath, themeGlobalConfigPath) { const updatedCommentJson = fs.readFileSync(themeGlobalConfigPath, 'utf-8'); - const originalCommentJson = fs.readFileSync(userDefinedGlobalConfigPath, 'utf-8'); + const originalCommentJson = fs.readFileSync(userGlobalConfigPath, 'utf-8'); return mergeJson(updatedCommentJson, originalCommentJson); } diff --git a/tests/postupgrade/config3.json b/tests/postupgrade/config3.json index c1c442951..b9c98c97f 100644 --- a/tests/postupgrade/config3.json +++ b/tests/postupgrade/config3.json @@ -5,4 +5,4 @@ // Comment2 , "two": "bye" -} +} \ No newline at end of file diff --git a/tests/postupgrade/utils.js b/tests/postupgrade/utils.js index 0ecb02cb4..3275840d5 100644 --- a/tests/postupgrade/utils.js +++ b/tests/postupgrade/utils.js @@ -1,6 +1,4 @@ -const { - mergeJson -} = require('postupgrade/utils.js'); +const { mergeJson } = require('postupgrade/utils.js'); const fs = require('fs'); describe('postupgrade utils', () => { @@ -8,7 +6,7 @@ describe('postupgrade utils', () => { it('puts together two comment JSON strings', () => { const config1 = fs.readFileSync('./tests/postupgrade/config1.json', 'utf-8'); const config2 = fs.readFileSync('./tests/postupgrade/config2.json', 'utf-8'); - const expected = fs.readFileSync('./tests/postupgrade/config3.json', 'utf-8').trim(); + const expected = fs.readFileSync('./tests/postupgrade/config3.json', 'utf-8'); // if this passes, then it is correctly not deleting any fields or comments // and it is giving precedence to the second parameter config