fix(aws-autoscaling): notificationTargetArn should be optional in LifecycleHook

[comcalvi]

This makes the notificationTargetArn optional in LifecycleHook. CloudFormation docs specify it as optional here. Closes #14641.
To achieve this, the role parameter was made optional. To avoid breaking users, a role is provided if users specify a notificationTarget (which they currently all do, as it is a required property) and is not provided otherwise.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[rix0rrr]

Default is written is @default - No target. This comment applies multiple times.

I guess add a line explaining why this is useful...?

[rix0rrr]

Why is it useful to not have a target? What does that do?

[comcalvi]

The CloudFormation docs specify both role and the target as optional parameters, so to be consistent with CloudFormation it should be optional. Beyond that though, unless a user wants notifications there's no reason to specify a target or a role, because those two parameters exist only to allow for notifications.

But when they use CDK, users have to specify a notificationTarget, even if they don't need to set up notifications.

[rix0rrr]

Leftover

[rix0rrr]

I don't think this should become read/write.

I know the API doesn't let you do what you need to do, which sucks. We'll have to find a way around that, but this is not the right way. We're going to either (a) have to break the api or (b) do type hackery.

Let's do (a), this once.

[comcalvi]

Do you mean removing this logic

private readonly _role?: iam.IRole;

public get role(): iam.IRole {
  if (!this._role) { throw new Error('Oh no we don\'t have a role'); }
  return this._role;
}

and going back to having public readonly role?: iam.IRole; like we had previously?

[rix0rrr]

Let me clarify here, so we don't spread the discussion around different places.

It looks like the reason your made this writable is so that the hooks can reassign the value in the bind() method, in the knowledge that this gets used later on. This is a contract, of sorts, but I don't like it very much because it's not obvious that's what's happening or that's what you need to do.

Any time we get by by mutating some variable somewhere in a particular order, I get extremely frowny because those kinds of ordering constraints are non-obvious, and cannot be compiler-checked (and are therefore easy to accidentally break).

Instead, by changing some types (and yes, eating the breakage that comes with that) we can also achieve the same declaratively. I outlined a better solution here: #16187 (comment)

[rix0rrr]

Ew :/

How about we change the signature of bind() to:

public bind(scope: Construct, options: BindHookTargetOptions): BindHookTargetResult;

Then we will have:

interface BindHookTargetOptions {
  readonly lifecycleHook: autoscaling.ILifecycleHook;
  readonly role?: iam.IRole;
}

interface BindHookTargetResult {
  readonly createdRole?: iam.IRole;
}

The linter will yell at you that it's a breaking change, but you have my permission to silence it (by adding it to allowed-breaking-changes.txt).

[rix0rrr]

Thanks! Conditional approval based on a tiny change.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 60131cc
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).