fix(aws-autoscaling): notificationTargetArn should be optional in LifecycleHook#16187
Conversation
…the policies in this test case (no notification arn)
This reverts commit fac2b68.
| /** | ||
| * The target of the lifecycle hook | ||
| * | ||
| * @default: No target. |
There was a problem hiding this comment.
Default is written is @default - No target. This comment applies multiple times.
I guess add a line explaining why this is useful...?
There was a problem hiding this comment.
Why is it useful to not have a target? What does that do?
There was a problem hiding this comment.
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.
| }); | ||
|
|
||
| const targetProps = props.notificationTarget.bind(this, this); | ||
| //let targetProps = undefined; |
packages/@aws-cdk/aws-autoscaling-hooktargets/lib/queue-hook.ts
Outdated
Show resolved
Hide resolved
| * Otherwise, no role is created. | ||
| */ | ||
| readonly role: iam.IRole; | ||
| role: iam.IRole; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
| const topic = new sns.Topic(_scope, 'Topic', { | ||
| masterKey: this.encryptionKey, | ||
| }); | ||
| try { lifecycleHook.role; } catch (noRoleError) { |
There was a problem hiding this comment.
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).
924c117 to
ebfd5f2
Compare
00c8c39 to
8815c83
Compare
867a3f9 to
4c5f614
Compare
rix0rrr
left a comment
There was a problem hiding this comment.
Thanks! Conditional approval based on a tiny change.
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
…ecycleHook (aws#16187) This makes the notificationTargetArn optional in LifecycleHook. CloudFormation docs specify it as optional [here](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-as-lifecyclehook.html). Closes aws#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*
This makes the notificationTargetArn optional in LifecycleHook. CloudFormation docs specify it as optional here. Closes #14641.
To achieve this, the
roleparameter was made optional. To avoid breaking users, a role is provided if users specify anotificationTarget(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