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

(ec2): Unexpected signature change for IVolume.grantAttachVolumeByResourceTag #22975

Closed
zessx opened this issue Nov 18, 2022 · 8 comments · Fixed by aws/jsii#3848
Closed

(ec2): Unexpected signature change for IVolume.grantAttachVolumeByResourceTag #22975

zessx opened this issue Nov 18, 2022 · 8 comments · Fixed by aws/jsii#3848
Assignees
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. jsii This issue originates in jsii, or this feature must be implemented in jsii. p1

Comments

@zessx
Copy link

zessx commented Nov 18, 2022

Describe the bug

The IVolume.grantAttachVolumeByResourceTag signature seems to have changed, as it no longer accept a list of Construct in the contructs argument.

Still, no trace of any change in the documentation or in the CHANGELOG.

Expected Behavior

This code is expected to work, like with the version 2.50.0:

volume.grant_attach_volume_by_resource_tag(instance.grant_principal, [instance])

Current Behavior

This code now raises the following error:

Traceback (most recent call last):
  File "…/test.py", line 39, in <module>
    DemoStack(app)
  File "…/.venv/lib/python3.10/site-packages/jsii/_runtime.py", line 109, in __call__
    inst = super().__call__(*args, **kwargs)
  File "…/test.py", line 36, in __init__
    volume.grant_attach_volume_by_resource_tag(instance.grant_principal, [instance])
  File "…/.venv/lib/python3.10/site-packages/aws_cdk/aws_ec2/__init__.py", line 77309, in grant_attach_volume_by_resource_tag
    constructs: typing.Sequence[constructs.Construct],
AttributeError: 'list' object has no attribute 'Construct'

Reproduction Steps

#!/usr/bin/env python3

from constructs import Construct
from aws_cdk import (
    App,
    Environment,
    Size,
    Stack,
    aws_ec2 as ec2
)

AWS_ACCOUNT = "…"
AWS_REGION = "…"

class DemoStack(Stack):

    def __init__(self, scope: Construct) -> None:
        super().__init__(scope, "DemoStack",
            env = Environment(
                account = AWS_ACCOUNT,
                region = AWS_REGION))

        vpc = ec2.Vpc(self, "Vpc")

        instance = ec2.Instance(self, "Instance",
            instance_type = ec2.InstanceType.of(
                instance_class = ec2.InstanceClass.BURSTABLE3,
                instance_size = ec2.InstanceSize.NANO),
            machine_image = ec2.MachineImage.generic_linux({
                AWS_REGION: "ami-0126a8e63ad208ba5"}),
            vpc = vpc)

        volume = ec2.Volume(self, "Volume",
            availability_zone = vpc.availability_zones[0],
            size = Size.gibibytes(10))

        # Working in 2.50.0, not in 2.51.0
        volume.grant_attach_volume_by_resource_tag(instance.grant_principal, [instance])

app = App()
DemoStack(app)

Works with 2.50.0, fails with 2.51.0.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.51.0

Framework Version

No response

Node.js Version

v14.17.6

OS

macOS 12.3 (21E230)

Language

Python

Language Version

Python 3.10.8

Other information

No response

@zessx zessx added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 18, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Nov 18, 2022
@peterwoodworth
Copy link
Contributor

Thanks for reporting this, I was able to reproduce this in Python but not in TypeScript, which leads me to believe this might be a JSII regression. We'll look into fixing this soon

@peterwoodworth peterwoodworth added p1 jsii This issue originates in jsii, or this feature must be implemented in jsii. and removed needs-triage This issue or PR still needs to be triaged. labels Nov 18, 2022
@RomainMuller
Copy link
Contributor

Confirmed what @peterwoodworth is saying... This is a bug in the jsii generated code that stems from the fact the constructs parameter shadows the constructs library in this particular context, which results in... not what's intended.

RomainMuller added a commit to aws/jsii that referenced this issue Nov 21, 2022
In certain cases, a function parameter may shadow an imported module
name, resultin in run-time errors either during type-checking or during
the type-cast that is performed before returning a kernel call's result.

This change adds a test case that covers this particular scenario in
Python, and changes how foreign modules are imported so that an alias
is always generated for those, removin the risk for collisions.

Fixes aws/aws-cdk#22975
mergify bot pushed a commit to aws/jsii that referenced this issue Nov 22, 2022
In certain cases, a function parameter may shadow an imported module name, resultin in run-time errors either during type-checking or during the type-cast that is performed before returning a kernel call's result.

This change adds a test case that covers this particular scenario in Python, and changes how foreign modules are imported so that an alias is always generated for those, removing the risk for collisions. It also moves the type-checking stubs out to the root of the module to remove the risk of more "runtime context" polluting the type evaluation.

Fixes aws/aws-cdk#22975



---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@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.

@zessx
Copy link
Author

zessx commented Nov 23, 2022

@RomainMuller As JSII versions are fixed in package.json, I'd leave this issue opened while the JSII release is not done yet, and used by CDK.

@peterwoodworth
Copy link
Contributor

We always close on merge of code, there is going to be some delay between that and the next release on any issue that gets fixed

@zessx
Copy link
Author

zessx commented Dec 13, 2022

Merged in JSII 1.72.0

@zessx
Copy link
Author

zessx commented Dec 15, 2022

And finally deployed with CDK 2.55.0, thanks :)

@peterwoodworth
Copy link
Contributor

This was an abnormally long wait @zessx, thanks a ton for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. jsii This issue originates in jsii, or this feature must be implemented in jsii. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants