Parameter change approval #104

jpb · 2018-04-13T01:23:21Z

Adds:

iidy param set --with-approval
iidy param review ...
Metadata to iidy get-history

Fixes:

bug with iidy get-history which omitted the first previous value

Closes #91

pauloancheta · 2018-04-16T01:05:36Z

src/params/index.ts

+      await setParamTags(ssm, Name, tags);
+      return 0;
+    } else {
+      return 130;


I know we use a lot of numbers in the repo. Wondering if these numbers should be constants that live somewhere so it can be clearer. Maybe we can also prepend it with SH to signify that it's the shell exit code that it returns?

return SH_SUCCESS // for 0 returns return SH_FAILURE // for 1 returns return SH_TERMINATED // I'm assuming 130 happens when we press <Ctrl-c>?

Plus one for less magic numbers

pauloancheta · 2018-04-16T01:15:15Z

src/params/index.ts

+    const Overwrite = true;
+    const KeyId = Type === 'SecureString' ? await getKMSAliasForParameter(Name) : undefined;
+
+    console.log(`Current: ${currentValue}`);


could we use this here? https://github.com/unbounce/iidy/blob/master/src/logger.ts#L13

Fix "previous" list to include correct entries. Add some metadata to 'simple' format

tavisrudd · 2018-04-16T21:08:33Z

src/params/index.ts

@@ -74,8 +75,8 @@ export async function setParam(argv: SetParamArgs): Promise<number> {
  const res = await ssm.putParameter({Name, Value, Type, KeyId, Overwrite}).promise();

  if(argv.withApproval) {
-    console.log('Parameter change is pending approval. Review change with:');
-    console.log(`  iidy --region ${argv.region} param review ${argv.path}`);
+    logger.info('Parameter change is pending approval. Review change with:');


Are we also using logger.info on the template approval side?

tavisrudd · 2018-04-16T21:18:32Z

This looks good, but I think we should remove the use of logger.info and switch back to console.log. That's what we're using elsewhere and the info: prefix adds noise to the output, especially for commands like get and get-history which can be told to output json for parsing / chaining with other tools.

There's also a bug in pagination that I'll look into further:

$ LOG_IIDY_ERROR=1 iidy param --profile sandbox get-by-path --recursive /
error: this.makeRequest is not a function
error:  TypeError: this.makeRequest is not a function
    at svc.(anonymous function) (/Users/tavis/src/ub/cf_templates/node_modules/aws-sdk/lib/service.js:499:23)
    at Object.paginateAwsCall (/Users/tavis/src/ub/cf_templates/lib/paginateAwsCall.js:6:22)
    at Object.getParamsByPath (/Users/tavis/src/ub/cf_templates/lib/params/index.js:175:55)
    at <anonymous>

tavisrudd · 2018-04-16T21:28:17Z

src/params/index.ts

@@ -108,8 +196,7 @@ export async function _getParamsByPath(Path: string): Promise<aws.SSM.ParameterL
    Path,
    WithDecryption: true
  };
-  const parameters: aws.SSM.ParameterList = await paginateAwsCall(
-    (args) => ssm.getParametersByPath(args), args, 'Parameters');
+  const parameters: aws.SSM.ParameterList = await paginateAwsCall(ssm.getParametersByPath, args, 'Parameters');


This introduces a bug. We should either revert or bind.

error: this.makeRequest is not a function error: TypeError: this.makeRequest is not a function at svc.(anonymous function) (/Users/tavis/src/ub/cf_templates/node_modules/aws-sdk/lib/service.js:499:23) at Object.paginateAwsCall (/Users/tavis/src/ub/cf_templates/lib/paginateAwsCall.js:6:22) at Object.getParamsByPath (/Users/tavis/src/ub/cf_templates/lib/params/index.js:175:55) at <anonymous>

tavisrudd · 2018-04-16T21:28:34Z

src/params/index.ts

@@ -121,43 +208,60 @@ export async function getParamsByPath(argv: GetParamsByPathArgs): Promise<number
    Recursive: argv.recursive,
    WithDecryption: argv.decrypt
  };
-  const parameters: aws.SSM.ParameterList = await paginateAwsCall((args) => ssm.getParametersByPath(args), args, 'Parameters');
+  const parameters: aws.SSM.ParameterList = await paginateAwsCall(ssm.getParametersByPath, args, 'Parameters');


See above. This is broken.

tavisrudd · 2018-04-16T21:32:17Z

src/statusCodes.ts

@@ -0,0 +1,3 @@
+export const SUCCESS = 1;


These are backwards in shell terms. 0=SUCCESS

tavisrudd · 2018-04-16T21:35:49Z

src/params/index.ts

+    return res && res.Parameter;
+  } catch(e) {
+    // Return undefined if parameter does not exist
+    if(!(e.code && e.code === 'ParameterNotFound')) {


This would read better if we were to explicitly return undefined in the true case and throw in the else.

tavisrudd · 2018-04-16T21:37:33Z

src/params/index.ts

  return ssm.listTagsForResource({ResourceId: path, ResourceType: 'Parameter'})
    .promise()
    .then((res) => _.fromPairs(_.map(res.TagList, (tag) => [tag.Key, tag.Value])));
 }

-async function mergeParamTags(param: aws.SSM.Parameter) {
-  return _.merge({}, param, {Tags: await getParamTags(param.Name!)});
+async function mergeParamTags<T extends aws.SSM.Parameter|aws.SSM.ParameterHistory>(ssm: aws.SSM, param: T) {


Don't forget to run tsfmt prior to pushing. It inserts spaces around |.

tavisrudd · 2018-04-16T22:36:21Z

src/params/index.ts

@@ -121,43 +209,60 @@ export async function getParamsByPath(argv: GetParamsByPathArgs): Promise<number
    Recursive: argv.recursive,
    WithDecryption: argv.decrypt
  };
-  const parameters: aws.SSM.ParameterList = await paginateAwsCall((args) => ssm.getParametersByPath(args), args, 'Parameters');
+  const parameters: aws.SSM.ParameterList = await paginateAwsCall((p) => ssm.getParametersByPath(p), args, 'Parameters');


p is a bit misleading here as the argument is actually the same args that are passed in later in the call rather than a path.

tavisrudd · 2018-04-16T22:40:16Z

src/params/index.ts

+
+  if (argv.withApproval) {
+    console.log('Parameter change is pending approval. Review change with:');
+    console.log(`  iidy --region ${argv.region} param review ${argv.path}`);


argv.region will often be null. Instead, import getCurrentAWSRegion from '../getCurrentAWSRegion';...

e.g.

$ iidy param --profile sandbox set --with-approval /tavis blah2 Parameter change is pending approval. Review change with: iidy --region null param review /tavis

tavisrudd · 2018-04-16T23:04:38Z

Looks good now.

jpb changed the title ~~[WIP] Parameter change approval~~ Parameter change approval Apr 13, 2018

pauloancheta reviewed Apr 16, 2018

View reviewed changes

jpb added 3 commits April 16, 2018 20:04

Add param set --with-approval, --message, and param review

ae6e702

Clean up param history

ce12561

Fix "previous" list to include correct entries. Add some metadata to 'simple' format

Remove status code "magic numbers"

3c6bfc2

jpb force-pushed the param-approval branch from c71e228 to a223dfe Compare April 16, 2018 20:05

jpb requested a review from tavisrudd April 16, 2018 21:08

tavisrudd reviewed Apr 16, 2018

View reviewed changes

pauloancheta approved these changes Apr 16, 2018

View reviewed changes

Tags are not versioned, only display for "current"

9206a45

jpb force-pushed the param-approval branch from a223dfe to 9206a45 Compare April 16, 2018 21:23

tavisrudd reviewed Apr 16, 2018

View reviewed changes

tavisrudd requested changes Apr 16, 2018

View reviewed changes

src/statusCodes.ts Outdated

@@ -0,0 +1,3 @@

export const SUCCESS = 1;

Copy link

Collaborator

tavisrudd Apr 16, 2018

Choose a reason for hiding this comment

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

These are backwards in shell terms. 0=SUCCESS

tavisrudd reviewed Apr 16, 2018

View reviewed changes

Fix status codes

56a317e

jpb force-pushed the param-approval branch from bbad39a to a72ef14 Compare April 16, 2018 22:06

tavisrudd reviewed Apr 16, 2018

View reviewed changes

tavisrudd requested changes Apr 16, 2018

View reviewed changes

Format, apply review feedback

fa0061f

jpb force-pushed the param-approval branch from a72ef14 to fa0061f Compare April 16, 2018 22:52

tavisrudd approved these changes Apr 16, 2018

View reviewed changes

tavisrudd merged commit ede0293 into master Apr 16, 2018

tavisrudd deleted the param-approval branch April 16, 2018 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter change approval #104

Parameter change approval #104

jpb commented Apr 13, 2018 •

edited

Loading

pauloancheta Apr 16, 2018

geoffnettaglich Apr 16, 2018

pauloancheta Apr 16, 2018

tavisrudd Apr 16, 2018

tavisrudd commented Apr 16, 2018 •

edited

Loading

tavisrudd Apr 16, 2018

tavisrudd Apr 16, 2018

tavisrudd Apr 16, 2018

tavisrudd Apr 16, 2018 •

edited

Loading

tavisrudd Apr 16, 2018

tavisrudd Apr 16, 2018

tavisrudd Apr 16, 2018

tavisrudd Apr 16, 2018 •

edited

Loading

tavisrudd commented Apr 16, 2018

Parameter change approval #104

Parameter change approval #104

Conversation

jpb commented Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavisrudd commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavisrudd Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavisrudd Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

tavisrudd commented Apr 16, 2018

jpb commented Apr 13, 2018 •

edited

Loading

tavisrudd commented Apr 16, 2018 •

edited

Loading

tavisrudd Apr 16, 2018 •

edited

Loading

tavisrudd Apr 16, 2018 •

edited

Loading