Skip to content

Commit

Permalink
fix(toolkit): fix context passed in from command-line (#1939)
Browse files Browse the repository at this point in the history 
Fix issue where context passed in via command-line was ignored.

Migration behavior has been changed: context that looks like it
has been generated by context providers (keys containing ":")
will upon loading be migrated to `cdk.context.json`; other
context in `cdk.json` will be left as-is and treated as readonly.

Fixes #1911.
  • Loading branch information
@rix0rrr
rix0rrr authored Mar 4, 2019
1 parent e150648 commit 82d1862
Show file tree
Hide file tree
Showing 6 changed files with 183 additions and 66 deletions.
2 changes: 1 addition & 1 deletion packages/aws-cdk/lib/api/cxapp/exec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { SDK } from '../util/sdk';
export async function execProgram(aws: SDK, config: Configuration): Promise<cxapi.SynthesizeResponse> {
const env: { [key: string]: string } = { };

const context = config.context.everything();
const context = config.context.all;
await populateDefaultEnvironmentIfNeeded(aws, context);

let pathMetadata: boolean = config.settings.get(['pathMetadata']);
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/lib/commands/context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export function handler(args: yargs.Arguments) {
export async function realHandler(options: CommandOptions): Promise<number> {
const { configuration, args } = options;

const contextValues = configuration.context.everything();
const contextValues = configuration.context.all;

if (args.clear) {
configuration.context.clear();
Expand Down
112 changes: 79 additions & 33 deletions packages/aws-cdk/lib/settings.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ const CONTEXT_KEY = 'context';
*/
export class Configuration {
public settings = new Settings();
public context = new Context(new Settings(), [], new Settings());
public context = new Context();

private readonly defaultConfig = new Settings({ versionReporting: true, pathMetadata: true });
private readonly commandLineArguments: Settings;
private readonly commandLineContext: Settings;
private projectConfig: Settings;
private projectContext: Settings;
private loaded = false;
Expand All @@ -30,6 +31,7 @@ export class Configuration {
this.commandLineArguments = commandLineArguments
? Settings.fromCommandLineArguments(commandLineArguments)
: new Settings();
this.commandLineContext = this.commandLineArguments.subSettings([CONTEXT_KEY]).makeReadOnly();
}

/**
Expand All @@ -40,7 +42,12 @@ export class Configuration {
this.projectConfig = await loadAndLog(PROJECT_CONFIG);
this.projectContext = await loadAndLog(PROJECT_CONTEXT);

this.context = new Context(this.projectConfig, [CONTEXT_KEY], this.projectContext);
await this.migrateLegacyContext();

this.context = new Context(
this.commandLineContext,
this.projectConfig.subSettings([CONTEXT_KEY]).makeReadOnly(),
this.projectContext);

// Build settings from what's left
this.settings = this.defaultConfig
Expand All @@ -55,66 +62,103 @@ export class Configuration {
}

/**
* Save the project config
* Save the project context
*/
public async saveContext(): Promise<this> {
if (!this.loaded) { return this; }
if (!this.loaded) { return this; } // Avoid overwriting files with nothing

if (this.context.modifiedBottom) {
await this.projectConfig.save(PROJECT_CONFIG);
}
await this.projectContext.save(PROJECT_CONTEXT);

return this;
}

/**
* Migrate context from the 'context' field in the projectConfig object to the dedicated object
*
* Only migrate context whose key contains a ':', to migrate only context generated
* by context providers.
*/
private async migrateLegacyContext() {
const legacyContext = this.projectConfig.get([CONTEXT_KEY]);
if (legacyContext === undefined) { return; }

const toMigrate = Object.keys(legacyContext).filter(k => k.indexOf(':') > -1);
if (toMigrate.length === 0) { return; }

for (const key of toMigrate) {
this.projectContext.set([key], legacyContext[key]);
this.projectConfig.unset([CONTEXT_KEY, key]);
}

// If the source object is empty now, completely remove it
if (Object.keys(this.projectConfig.get([CONTEXT_KEY])).length === 0) {
this.projectConfig.unset([CONTEXT_KEY]);
}

// Save back
await this.projectConfig.save(PROJECT_CONFIG);
await this.projectContext.save(PROJECT_CONTEXT);
}
}

async function loadAndLog(fileName: string): Promise<Settings> {
const ret = new Settings();
await ret.load(fileName);
if (!ret.empty) {
debug(fileName + ':', JSON.stringify(ret.get([]), undefined, 2));
debug(fileName + ':', JSON.stringify(ret.all, undefined, 2));
}
return ret;
}

/**
* Class that supports overlaying 2 property bags
* Class that supports overlaying property bags
*
* Writes go to the topmost property bag, but if any writes collide between
* them the value will be deleted from the underlying property bag.
* Reads come from the first property bag that can has the given key,
* writes go to the first property bag that is not readonly. A write
* will remove the value from all property bags after the first
* writable one.
*/
export class Context {
public modifiedBottom = false;
private readonly bags: Settings[];

constructor(private readonly bottom: Settings, private readonly bottomPrefixPath: string[], private readonly top: Settings) {
constructor(...bags: Settings[]) {
this.bags = bags.length > 0 ? bags : [new Settings()];
}

public get keys(): string[] {
return Object.keys(this.everything());
return Object.keys(this.all);
}

public has(key: string) {
return this.keys.indexOf(key) > -1;
}

public everything(): {[key: string]: any} {
const b = this.bottom.get(this.bottomPrefixPath) || {};
const t = this.top.get([]) || {};
return Object.assign(b, t);
public get all(): {[key: string]: any} {
let ret = new Settings();

// In reverse order so keys to the left overwrite keys to the right of them
for (const bag of [...this.bags].reverse()) {
ret = ret.merge(bag);
}

return ret.all;
}

public get(key: string): any {
let x = this.top.get([key]);
if (x === undefined) { x = this.bottom.get(this.bottomPrefixPath.concat([key])); }
return x;
for (const bag of this.bags) {
const v = bag.get([key]);
if (v !== undefined) { return v; }
}
return undefined;
}

public set(key: string, value: any) {
this.top.set([key], value);
if (this.bottom.get(this.bottomPrefixPath.concat([key])) !== undefined) {
this.bottom.unset(this.bottomPrefixPath.concat([key]));
this.modifiedBottom = true;
for (const bag of this.bags) {
if (bag.readOnly) { continue; }

// All bags past the first one have the value erased
bag.set([key], value);
value = undefined;
}
}

Expand Down Expand Up @@ -177,7 +221,7 @@ export class Settings {
return ret;
}

constructor(private settings: SettingsMap = {}, private readOnly = false) {}
constructor(private settings: SettingsMap = {}, public readonly readOnly = false) {}

public async load(fileName: string): Promise<this> {
if (this.readOnly) {
Expand Down Expand Up @@ -205,14 +249,20 @@ export class Settings {
return this;
}

public get all(): any {
return this.get([]);
}

public merge(other: Settings): Settings {
return new Settings(util.deepMerge(this.settings, other.settings));
}

public subSettings(keyPrefix: string[]) {
return new Settings(this.get(keyPrefix) || {}, false);
}

public makeReadOnly(): Settings {
const ret = this.clone();
ret.readOnly = true;
return ret;
return new Settings(this.settings, true);
}

public clear() {
Expand Down Expand Up @@ -247,10 +297,6 @@ export class Settings {
this.set(path, undefined);
}

public clone() {
return new Settings({ ...this.settings });
}

private prohibitContextKey(key: string, fileName: string) {
if (!this.settings.context) { return; }
if (key in this.settings.context) {
Expand Down
7 changes: 6 additions & 1 deletion packages/aws-cdk/test/commands/test.context-command.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,19 @@ export = {
configuration.context.set('foo', 'bar');
configuration.context.set('baz', 'quux');

test.deepEqual(configuration.context.all, {
foo: 'bar',
baz: 'quux'
});

// WHEN
await realHandler({
configuration,
args: { reset: 'foo' }
} as any);

// THEN
test.deepEqual(configuration.context.everything(), {
test.deepEqual(configuration.context.all, {
baz: 'quux'
});

Expand Down
54 changes: 24 additions & 30 deletions packages/aws-cdk/test/test.context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,41 +43,24 @@ export = {
test.done();
},

async 'new context always goes to dedicated file, other context stays in source file'(test: Test) {
async 'context with colons gets migrated to new file'(test: Test) {
// GIVEN
await fs.writeJSON('cdk.context.json', { foo: 'bar' });
await fs.writeJSON('cdk.json', { context: { boo: 'far' } });
await fs.writeJSON('cdk.json', { context: { 'boo': 'far', 'boo:boo': 'far:far' } });
const config = await new Configuration().load();

// WHEN
config.context.set('baz', 'quux');
await config.saveContext();

// THEN
test.deepEqual(await fs.readJSON('cdk.context.json'), { foo: 'bar', baz: 'quux' });
test.deepEqual(await fs.readJSON('cdk.json'), { context: { boo: 'far' } });
test.deepEqual(await fs.readJSON('cdk.context.json'), { 'foo': 'bar', 'boo:boo': 'far:far', 'baz': 'quux' });
test.deepEqual(await fs.readJSON('cdk.json'), { context: { boo: 'far'} });

test.done();
},

async 'overwritten always goes to dedicated file, removed from source file'(test: Test) {
// GIVEN
await fs.writeJSON('cdk.context.json', { foo: 'bar' });
await fs.writeJSON('cdk.json', { context: { foo: 'bar' } });
const config = await new Configuration().load();

// WHEN
config.context.set('foo', 'boom');
await config.saveContext();

// THEN
test.deepEqual(await fs.readJSON('cdk.context.json'), { foo: 'boom' });
test.deepEqual(await fs.readJSON('cdk.json'), { context: {} });

test.done();
},

async 'deleted context disappears from both files'(test: Test) {
async 'deleted context disappears from new file'(test: Test) {
// GIVEN
await fs.writeJSON('cdk.context.json', { foo: 'bar' });
await fs.writeJSON('cdk.json', { context: { foo: 'bar' } });
Expand All @@ -89,12 +72,12 @@ export = {

// THEN
test.deepEqual(await fs.readJSON('cdk.context.json'), {});
test.deepEqual(await fs.readJSON('cdk.json'), { context: {} });
test.deepEqual(await fs.readJSON('cdk.json'), { context: { foo: 'bar' }});

test.done();
},

async 'clear deletes from both files'(test: Test) {
async 'clear deletes from new file'(test: Test) {
// GIVEN
await fs.writeJSON('cdk.context.json', { foo: 'bar' });
await fs.writeJSON('cdk.json', { context: { boo: 'far' } });
Expand All @@ -106,23 +89,23 @@ export = {

// THEN
test.deepEqual(await fs.readJSON('cdk.context.json'), {});
test.deepEqual(await fs.readJSON('cdk.json'), { context: {} });
test.deepEqual(await fs.readJSON('cdk.json'), { context: { boo: 'far' } });

test.done();
},

async 'surive missing new file'(test: Test) {
// GIVEN
await fs.writeJSON('cdk.json', { context: { boo: 'far' } });
await fs.writeJSON('cdk.json', { context: { 'boo:boo' : 'far' } });
const config = await new Configuration().load();

// WHEN
test.deepEqual(config.context.everything(), { boo: 'far' });
test.deepEqual(config.context.all, { 'boo:boo' : 'far' });
await config.saveContext();

// THEN
test.deepEqual(await fs.readJSON('cdk.context.json'), {});
test.deepEqual(await fs.readJSON('cdk.json'), { context: { boo: 'far' } });
test.deepEqual(await fs.readJSON('cdk.context.json'), { 'boo:boo' : 'far' });
test.deepEqual(await fs.readJSON('cdk.json'), {});

test.done();
},
Expand All @@ -134,12 +117,23 @@ export = {
const config = await new Configuration().load();

// WHEN
test.deepEqual(config.context.everything(), { boo: 'far' });
test.deepEqual(config.context.all, { boo: 'far' });
await config.saveContext();

// THEN
test.deepEqual(await fs.readJSON('cdk.context.json'), { boo: 'far' });

test.done();
},

async 'command line context is merged with stored context'(test: Test) {
// GIVEN
await fs.writeJSON('cdk.context.json', { boo: 'far' });
const config = await new Configuration({ context: ['foo=bar'] } as any).load();

// WHEN
test.deepEqual(config.context.all, { foo: 'bar', boo: 'far' });

test.done();
},
};
Loading

0 comments on commit 82d1862

Please sign in to comment.