From 715ce52bbfd9315757053b70fa6ebcf0dafb4871 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Thu, 6 Jun 2019 11:38:13 +0100 Subject: [PATCH 1/5] fix(aws-cdk): Move version check TTL file to home directory Originally, the version check TTL file was located alongside the CDK node installation directory. This caused issues for people who had CDK installed on a readonly filesystem. This change moves this file to the user's home directory, specifically under $HOMEDIR/.cdk/cache. This directory is already being used by account-cache.ts. The old logic is maintained where a repo check is run once a day against NPM, when a CDK command is invoked, and when a new version is present will generate a console banner notifying the user of this. The edge case with the updated implementation is when there are 2+ active installations of CDK and they are used interchangably. When this happens, the user will only get one notification per non-latest installation of CDK per day. --- packages/aws-cdk/.gitignore | 1 - packages/aws-cdk/lib/version.ts | 55 +++++++++++++++++++-------- packages/aws-cdk/test/test.version.ts | 38 +++++++++++++++--- 3 files changed, 71 insertions(+), 23 deletions(-) diff --git a/packages/aws-cdk/.gitignore b/packages/aws-cdk/.gitignore index 00f5e67ea2b57..93f15b0ddeed7 100644 --- a/packages/aws-cdk/.gitignore +++ b/packages/aws-cdk/.gitignore @@ -7,7 +7,6 @@ dist # Generated by generate.sh build-info.json -.LAST_VERSION_CHECK .LAST_BUILD .nyc_output diff --git a/packages/aws-cdk/lib/version.ts b/packages/aws-cdk/lib/version.ts index 7d9933b6b8f03..13df478cc3fd4 100644 --- a/packages/aws-cdk/lib/version.ts +++ b/packages/aws-cdk/lib/version.ts @@ -1,6 +1,9 @@ import { exec as _exec } from 'child_process'; import colors = require('colors/safe'); -import { close as _close, open as _open, stat as _stat } from 'fs'; +import fs = require('fs'); +import { mkdirsSync } from 'fs-extra'; +import os = require('os'); +import path = require('path'); import semver = require('semver'); import { promisify } from 'util'; import { debug, print, warning } from '../lib/logging'; @@ -8,10 +11,11 @@ import { formatAsBanner } from '../lib/util/console-formatters'; const ONE_DAY_IN_SECONDS = 1 * 24 * 60 * 60; -const close = promisify(_close); +const close = promisify(fs.close); const exec = promisify(_exec); -const open = promisify(_open); -const stat = promisify(_stat); +const open = promisify(fs.open); +const stat = promisify(fs.stat); +const write = promisify(fs.write); export const DISPLAY_VERSION = `${versionNumber()} (build ${commit()})`; @@ -23,15 +27,31 @@ function commit(): string { return require('../build-info.json').commit; } -export class TimestampFile { +export class VersionCheckTTL { + public static timestampFilePath(): string { + // Get the home directory from the OS, first. Fallback to $HOME. + const homedir = os.userInfo().homedir || os.homedir(); + if (!homedir || !homedir.trim()) { + throw new Error('Cannot determine home directory'); + } + // Using the same path from account-cache.ts + return path.join(homedir, '.cdk', 'cache', 'repo-version-ttl'); + } + private readonly file: string; - // File modify times are accurate only till the second, hence using seconds as precision + // File modify times are accurate only to the second private readonly ttlSecs: number; - constructor(file: string, ttlSecs: number) { - this.file = file; - this.ttlSecs = ttlSecs; + constructor(file?: string, ttlSecs?: number) { + this.file = file || VersionCheckTTL.timestampFilePath(); + try { + mkdirsSync(path.dirname(this.file)); + fs.accessSync(path.dirname(this.file), fs.constants.W_OK); + } catch { + throw new Error(`Directory (${path.dirname(this.file)}) is not writable.`); + } + this.ttlSecs = ttlSecs || ONE_DAY_IN_SECONDS; } public async hasExpired(): Promise { @@ -39,7 +59,7 @@ export class TimestampFile { const lastCheckTime = (await stat(this.file)).mtimeMs; const today = new Date().getTime(); - if ((today - lastCheckTime) / 1000 > this.ttlSecs) { // convert ms to secs + if ((today - lastCheckTime) / 1000 > this.ttlSecs) { // convert ms to sec return true; } return false; @@ -52,15 +72,19 @@ export class TimestampFile { } } - public async update(): Promise { + public async update(latestVersion?: string): Promise { const fd = await open(this.file, 'w'); + if (latestVersion && latestVersion.trim()) { + // writing to a file for debugging or manual check purposes only + await write(fd, latestVersion); + } await close(fd); } } // Export for unit testing only. // Don't use directly, use displayVersionMessage() instead. -export async function latestVersionIfHigher(currentVersion: string, cacheFile: TimestampFile): Promise { +export async function latestVersionIfHigher(currentVersion: string, cacheFile: VersionCheckTTL): Promise { if (!(await cacheFile.hasExpired())) { return null; } @@ -74,7 +98,7 @@ export async function latestVersionIfHigher(currentVersion: string, cacheFile: T throw new Error(`npm returned an invalid semver ${latestVersion}`); } const isNewer = semver.gt(latestVersion, currentVersion); - await cacheFile.update(); + await cacheFile.update(latestVersion); if (isNewer) { return latestVersion; @@ -83,14 +107,13 @@ export async function latestVersionIfHigher(currentVersion: string, cacheFile: T } } -const versionCheckCache = new TimestampFile(`${__dirname}/../.LAST_VERSION_CHECK`, ONE_DAY_IN_SECONDS); - export async function displayVersionMessage(): Promise { if (!process.stdout.isTTY) { return; } try { + const versionCheckCache = new VersionCheckTTL(); const laterVersion = await latestVersionIfHigher(versionNumber(), versionCheckCache); if (laterVersion) { const bannerMsg = formatAsBanner([ @@ -100,6 +123,6 @@ export async function displayVersionMessage(): Promise { bannerMsg.forEach((e) => print(e)); } } catch (err) { - warning(`Could not run version check due to error ${err.message}`); + warning(`Could not run version check - ${err.message}`); } } \ No newline at end of file diff --git a/packages/aws-cdk/test/test.version.ts b/packages/aws-cdk/test/test.version.ts index e0df94382b900..ad68a6a2ef358 100644 --- a/packages/aws-cdk/test/test.version.ts +++ b/packages/aws-cdk/test/test.version.ts @@ -1,7 +1,9 @@ import { Test } from 'nodeunit'; +import os = require('os'); +import sinon = require('sinon'); import { setTimeout as _setTimeout } from 'timers'; import { promisify } from 'util'; -import { latestVersionIfHigher, TimestampFile } from '../lib/version'; +import { latestVersionIfHigher, VersionCheckTTL } from '../lib/version'; const setTimeout = promisify(_setTimeout); @@ -10,14 +12,27 @@ function tmpfile(): string { } export = { + 'tearDown'(callback: () => void) { + sinon.restore(); + callback(); + }, + + 'initialization fails on unwritable directory'(test: Test) { + test.expect(1); + test.throws(() => new VersionCheckTTL('/cdk-test/cache'), /not writable/); + test.done(); + }, + async 'cache file responds correctly when file is not present'(test: Test) { - const cache = new TimestampFile(tmpfile(), 1); + test.expect(1); + const cache = new VersionCheckTTL(tmpfile(), 1); test.strictEqual(await cache.hasExpired(), true); test.done(); }, async 'cache file honours the specified TTL'(test: Test) { - const cache = new TimestampFile(tmpfile(), 1); + test.expect(2); + const cache = new VersionCheckTTL(tmpfile(), 1); await cache.update(); test.strictEqual(await cache.hasExpired(), false); await setTimeout(1001); // Just above 1 sec in ms @@ -26,14 +41,16 @@ export = { }, async 'Skip version check if cache has not expired'(test: Test) { - const cache = new TimestampFile(tmpfile(), 100); + test.expect(1); + const cache = new VersionCheckTTL(tmpfile(), 100); await cache.update(); test.equal(await latestVersionIfHigher('0.0.0', cache), null); test.done(); }, async 'Return later version when exists & skip recent re-check'(test: Test) { - const cache = new TimestampFile(tmpfile(), 100); + test.expect(3); + const cache = new VersionCheckTTL(tmpfile(), 100); const result = await latestVersionIfHigher('0.0.0', cache); test.notEqual(result, null); test.ok((result as string).length > 0); @@ -44,9 +61,18 @@ export = { }, async 'Return null if version is higher than npm'(test: Test) { - const cache = new TimestampFile(tmpfile(), 100); + test.expect(1); + const cache = new VersionCheckTTL(tmpfile(), 100); const result = await latestVersionIfHigher('100.100.100', cache); test.equal(result, null); test.done(); }, + + 'No homedir for the given user'(test: Test) { + test.expect(1); + sinon.stub(os, 'homedir').returns(''); + sinon.stub(os, 'userInfo').returns({ username: '', uid: 10, gid: 11, shell: null, homedir: ''}); + test.throws(() => new VersionCheckTTL(), /Cannot determine home directory/); + test.done(); + } }; From d786634da830d24d458dcb60196697d1cd38ffa6 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 7 Jun 2019 10:52:31 +0100 Subject: [PATCH 2/5] Feedback from Pull Request * Drop use of fs and use fs-extra exclusively. * Cleaned up file write. * Fixed breaking unit test. https://github.com/awslabs/aws-cdk/pull/2774 --- packages/aws-cdk/lib/version.ts | 19 ++++++------------ packages/aws-cdk/test/test.version.ts | 28 +++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/packages/aws-cdk/lib/version.ts b/packages/aws-cdk/lib/version.ts index 13df478cc3fd4..23e70295a6e45 100644 --- a/packages/aws-cdk/lib/version.ts +++ b/packages/aws-cdk/lib/version.ts @@ -1,7 +1,6 @@ import { exec as _exec } from 'child_process'; import colors = require('colors/safe'); -import fs = require('fs'); -import { mkdirsSync } from 'fs-extra'; +import fs = require('fs-extra'); import os = require('os'); import path = require('path'); import semver = require('semver'); @@ -11,11 +10,7 @@ import { formatAsBanner } from '../lib/util/console-formatters'; const ONE_DAY_IN_SECONDS = 1 * 24 * 60 * 60; -const close = promisify(fs.close); const exec = promisify(_exec); -const open = promisify(fs.open); -const stat = promisify(fs.stat); -const write = promisify(fs.write); export const DISPLAY_VERSION = `${versionNumber()} (build ${commit()})`; @@ -46,7 +41,7 @@ export class VersionCheckTTL { constructor(file?: string, ttlSecs?: number) { this.file = file || VersionCheckTTL.timestampFilePath(); try { - mkdirsSync(path.dirname(this.file)); + fs.mkdirsSync(path.dirname(this.file)); fs.accessSync(path.dirname(this.file), fs.constants.W_OK); } catch { throw new Error(`Directory (${path.dirname(this.file)}) is not writable.`); @@ -56,7 +51,7 @@ export class VersionCheckTTL { public async hasExpired(): Promise { try { - const lastCheckTime = (await stat(this.file)).mtimeMs; + const lastCheckTime = (await fs.stat(this.file)).mtimeMs; const today = new Date().getTime(); if ((today - lastCheckTime) / 1000 > this.ttlSecs) { // convert ms to sec @@ -73,12 +68,10 @@ export class VersionCheckTTL { } public async update(latestVersion?: string): Promise { - const fd = await open(this.file, 'w'); - if (latestVersion && latestVersion.trim()) { - // writing to a file for debugging or manual check purposes only - await write(fd, latestVersion); + if (!latestVersion) { + latestVersion = ''; } - await close(fd); + fs.writeFileSync(this.file, latestVersion); } } diff --git a/packages/aws-cdk/test/test.version.ts b/packages/aws-cdk/test/test.version.ts index ad68a6a2ef358..4beb6ed3088d8 100644 --- a/packages/aws-cdk/test/test.version.ts +++ b/packages/aws-cdk/test/test.version.ts @@ -1,5 +1,7 @@ +import fs = require('fs-extra'); import { Test } from 'nodeunit'; import os = require('os'); +import path = require('path'); import sinon = require('sinon'); import { setTimeout as _setTimeout } from 'timers'; import { promisify } from 'util'; @@ -19,7 +21,9 @@ export = { 'initialization fails on unwritable directory'(test: Test) { test.expect(1); - test.throws(() => new VersionCheckTTL('/cdk-test/cache'), /not writable/); + const cacheFile = tmpfile(); + sinon.stub(fs, 'mkdirsSync').withArgs(path.dirname(cacheFile)).throws('Cannot make directory'); + test.throws(() => new VersionCheckTTL(cacheFile), /not writable/); test.done(); }, @@ -74,5 +78,25 @@ export = { sinon.stub(os, 'userInfo').returns({ username: '', uid: 10, gid: 11, shell: null, homedir: ''}); test.throws(() => new VersionCheckTTL(), /Cannot determine home directory/); test.done(); - } + }, + + 'Version specified is stored in the TTL file'(test: Test) { + test.expect(1); + const cacheFile = tmpfile(); + const cache = new VersionCheckTTL(cacheFile, 1); + cache.update('1.1.1'); + const storedVersion = fs.readFileSync(cacheFile, 'utf8'); + test.equal(storedVersion, '1.1.1'); + test.done(); + }, + + 'No Version specified for storage in the TTL file'(test: Test) { + test.expect(1); + const cacheFile = tmpfile(); + const cache = new VersionCheckTTL(cacheFile, 1); + cache.update(); + const storedVersion = fs.readFileSync(cacheFile, 'utf8'); + test.equal(storedVersion, ''); + test.done(); + }, }; From d419981d342a7012e44c51c25d17cb9f56041ff1 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 7 Jun 2019 13:20:51 +0100 Subject: [PATCH 3/5] Switched error messaging level to debug --- packages/aws-cdk/lib/version.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/version.ts b/packages/aws-cdk/lib/version.ts index 23e70295a6e45..8eab123e9ff7b 100644 --- a/packages/aws-cdk/lib/version.ts +++ b/packages/aws-cdk/lib/version.ts @@ -5,7 +5,7 @@ import os = require('os'); import path = require('path'); import semver = require('semver'); import { promisify } from 'util'; -import { debug, print, warning } from '../lib/logging'; +import { debug, print } from '../lib/logging'; import { formatAsBanner } from '../lib/util/console-formatters'; const ONE_DAY_IN_SECONDS = 1 * 24 * 60 * 60; @@ -116,6 +116,6 @@ export async function displayVersionMessage(): Promise { bannerMsg.forEach((e) => print(e)); } } catch (err) { - warning(`Could not run version check - ${err.message}`); + debug(`Could not run version check - ${err.message}`); } } \ No newline at end of file From debfb2c782e91cd2b5a167c9751bb3d19db91e7d Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 7 Jun 2019 16:01:41 +0200 Subject: [PATCH 4/5] Use async writeFile --- packages/aws-cdk/lib/version.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/version.ts b/packages/aws-cdk/lib/version.ts index 8eab123e9ff7b..8e4f9128b56e2 100644 --- a/packages/aws-cdk/lib/version.ts +++ b/packages/aws-cdk/lib/version.ts @@ -71,7 +71,7 @@ export class VersionCheckTTL { if (!latestVersion) { latestVersion = ''; } - fs.writeFileSync(this.file, latestVersion); + await fs.writeFile(this.file, latestVersion); } } @@ -118,4 +118,4 @@ export async function displayVersionMessage(): Promise { } catch (err) { debug(`Could not run version check - ${err.message}`); } -} \ No newline at end of file +} From fb67caa6defb03c1b9229af7b6212e438595bdae Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 7 Jun 2019 15:15:14 +0100 Subject: [PATCH 5/5] Fix failing tests --- packages/aws-cdk/test/test.version.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk/test/test.version.ts b/packages/aws-cdk/test/test.version.ts index 4beb6ed3088d8..d4c31b67be7b6 100644 --- a/packages/aws-cdk/test/test.version.ts +++ b/packages/aws-cdk/test/test.version.ts @@ -80,21 +80,21 @@ export = { test.done(); }, - 'Version specified is stored in the TTL file'(test: Test) { + async 'Version specified is stored in the TTL file'(test: Test) { test.expect(1); const cacheFile = tmpfile(); const cache = new VersionCheckTTL(cacheFile, 1); - cache.update('1.1.1'); + await cache.update('1.1.1'); const storedVersion = fs.readFileSync(cacheFile, 'utf8'); test.equal(storedVersion, '1.1.1'); test.done(); }, - 'No Version specified for storage in the TTL file'(test: Test) { + async 'No Version specified for storage in the TTL file'(test: Test) { test.expect(1); const cacheFile = tmpfile(); const cache = new VersionCheckTTL(cacheFile, 1); - cache.update(); + await cache.update(); const storedVersion = fs.readFileSync(cacheFile, 'utf8'); test.equal(storedVersion, ''); test.done();