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

aws_iam: any parameter type in AccountPrinciple constructor #20288

Closed
simonkarman opened this issue May 11, 2022 · 1 comment · Fixed by #20292
Closed

aws_iam: any parameter type in AccountPrinciple constructor #20288

simonkarman opened this issue May 11, 2022 · 1 comment · Fixed by #20292
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@simonkarman
Copy link

simonkarman commented May 11, 2022

Describe the bug

The AccountPrinciple class as found in: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-iam/lib/principals.ts#L395 has a construct that takes a single argument which is the accountId the type of this 'account id' parameter is any. Can this parameter be changed to be of type string instead?

Expected Behavior

I expect the following piece of code to give a Type error on the first parameter.

new aws_iam.AccountPrincipal({ hello: 'world' }); // This is currently valid, and should not be, it should throw a TypeError
new aws_iam.AccountPrincipal('0123456789012'); // This is valid, and should be

Current Behavior

Currently, TypeScript does not complain about the code below.

new aws_iam.AccountPrincipal({ hello: 'world' }); // This is currently valid, and should not be, it should throw a TypeError
new aws_iam.AccountPrincipal('0123456789012'); // This is valid, and should be

Reproduction Steps

import { aws_iam } from 'aws-cdk-lib';

new aws_iam.AccountPrincipal('0123456789012');
new aws_iam.AccountPrincipal({ hello: 'world' });

Possible Solution

To fix this we can simply replace any with string in the constructor parameter of the AccountPrinciple class in: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-iam/lib/principals.ts#L395

But first, we should double check there isn't any reasoning behind this being of type any.

Additional Information/Context

No response

CDK CLI Version

2.16.0 (build 4c77925)

Framework Version

No response

Node.js Version

v14.18.0

OS

MacOS

Language

Typescript

Language Version

3.9.7

Other information

No response

@simonkarman simonkarman added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 11, 2022
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label May 11, 2022
@mergify mergify bot closed this as completed in #20292 May 19, 2022
mergify bot pushed a commit that referenced this issue May 19, 2022
…20292)

Changed the type of accountId in AccountPrincipal constructor to string from any fixes #20288 


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

wphilipw pushed a commit to wphilipw/aws-cdk that referenced this issue May 23, 2022
…ws#20292)

Changed the type of accountId in AccountPrincipal constructor to string from any fixes aws#20288 


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants