Skip to content
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

fix(aws-cdk): Move version check TTL file to home directory #2774

Merged
merged 5 commits into from
Jun 11, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/aws-cdk/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ dist

# Generated by generate.sh
build-info.json
.LAST_VERSION_CHECK

.LAST_BUILD
.nyc_output
Expand Down
54 changes: 35 additions & 19 deletions packages/aws-cdk/lib/version.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
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-extra');
import os = require('os');
import path = require('path');
import semver = require('semver');
import { promisify } from 'util';
import { debug, print, warning } from '../lib/logging';
import { formatAsBanner } from '../lib/util/console-formatters';

const ONE_DAY_IN_SECONDS = 1 * 24 * 60 * 60;

const close = promisify(_close);
const exec = promisify(_exec);
const open = promisify(_open);
const stat = promisify(_stat);

export const DISPLAY_VERSION = `${versionNumber()} (build ${commit()})`;

Expand All @@ -23,23 +22,39 @@ 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 {
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.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this worth throwing? Shouldn't we flip a boolean that says "can't do any work, soz" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "can't do any work" message is printed by the catch block at the calling point of this constructor.

I did consider flipping a boolean and keeping it as a class member. This would mean the other methods on the class (like hasExpired()) return an additional state to the caller, which the caller will need to take care of.

Throwing an exception here seemed simpler and achieves the same experience.

}
this.ttlSecs = ttlSecs || ONE_DAY_IN_SECONDS;
}

public async hasExpired(): Promise<boolean> {
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 secs
if ((today - lastCheckTime) / 1000 > this.ttlSecs) { // convert ms to sec
return true;
}
return false;
Expand All @@ -52,15 +67,17 @@ export class TimestampFile {
}
}

public async update(): Promise<void> {
const fd = await open(this.file, 'w');
await close(fd);
public async update(latestVersion?: string): Promise<void> {
if (!latestVersion) {
latestVersion = '';
}
fs.writeFileSync(this.file, latestVersion);
}
}

// Export for unit testing only.
// Don't use directly, use displayVersionMessage() instead.
export async function latestVersionIfHigher(currentVersion: string, cacheFile: TimestampFile): Promise<string | null> {
export async function latestVersionIfHigher(currentVersion: string, cacheFile: VersionCheckTTL): Promise<string|null> {
if (!(await cacheFile.hasExpired())) {
return null;
}
Expand All @@ -74,7 +91,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;
Expand All @@ -83,14 +100,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<void> {
if (!process.stdout.isTTY) {
return;
}

try {
const versionCheckCache = new VersionCheckTTL();
const laterVersion = await latestVersionIfHigher(versionNumber(), versionCheckCache);
if (laterVersion) {
const bannerMsg = formatAsBanner([
Expand All @@ -100,6 +116,6 @@ export async function displayVersionMessage(): Promise<void> {
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}`);
}
}
62 changes: 56 additions & 6 deletions packages/aws-cdk/test/test.version.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
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';
import { latestVersionIfHigher, TimestampFile } from '../lib/version';
import { latestVersionIfHigher, VersionCheckTTL } from '../lib/version';

const setTimeout = promisify(_setTimeout);

Expand All @@ -10,14 +14,29 @@ function tmpfile(): string {
}

export = {
'tearDown'(callback: () => void) {
sinon.restore();
callback();
},

'initialization fails on unwritable directory'(test: Test) {
test.expect(1);
const cacheFile = tmpfile();
sinon.stub(fs, 'mkdirsSync').withArgs(path.dirname(cacheFile)).throws('Cannot make directory');
test.throws(() => new VersionCheckTTL(cacheFile), /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
Expand All @@ -26,14 +45,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);
Expand All @@ -44,9 +65,38 @@ 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();
},

'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();
},
};