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

rossetta issue: varargs are ignored when hoisting keyword arguments #1821

Closed
thomaspoignant opened this issue Jul 24, 2020 · 10 comments · Fixed by #1832
Closed

rossetta issue: varargs are ignored when hoisting keyword arguments #1821

thomaspoignant opened this issue Jul 24, 2020 · 10 comments · Fixed by #1832
Assignees
Labels
bug This issue is a bug. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort language/python Related to Python bindings

Comments

@thomaspoignant
Copy link

thomaspoignant commented Jul 24, 2020

Original title: cluster.add_resource got an unexpected keyword argument 'api_version'

Using Python when I try to add a resource using the example available here I have an error during the execution.

The error is :

TypeError: add_resource() got an unexpected keyword argument 'api_version'

Reproduction Steps

Create a stack with this

cluster = eks.Cluster(self, "hello-eks",
    version=eks.KubernetesVersion.V1_16
)

cluster.add_resource("mypod",
    api_version="v1",
    kind="Pod",
    metadata={"name": "mypod"},
    spec={
        "containers": [{
            "name": "hello",
            "image": "paulbouwer/hello-kubernetes:1.5",
            "ports": [{"container_port": 8080}]
        }
        ]
    }
)

and run cdk synth.

Error Log

TypeError: add_resource() got an unexpected keyword argument 'api_version'

Environment

  • CLI Version: aws-cli/2.0.33 Python/3.8.5 Darwin/19.6.0 botocore/2.0.0dev37
  • Framework Version: 1.54.0 (build c01b9b9)
  • Node.js Version: v14.5.0
  • OS : Mac OSX 10.15.6*
  • Language (Version): Python 3.8.5

This is 🐛 Bug Report

@eladb
Copy link
Contributor

eladb commented Jul 26, 2020

The add_resource API accepts a variable number of manifest arguments as objects and not through keyword arguments:

cluster.add_resource('MyResource', {
    "api_version": "v1",
    "kind": "Pod",
    "metadata": {"name": "mypod"},
    "spec": {
        "containers": [{
            "name": "hello",
            "image": "paulbouwer/hello-kubernetes:1.5",
            "ports": [{"container_port": 8080}]
        }]
    }
})

@eladb eladb closed this as completed Jul 26, 2020
@thomaspoignant
Copy link
Author

thomaspoignant commented Jul 26, 2020

@eladb I understand that you closed the issue, but the code snippet is directly from the documentation (see the link in my previous comment).

https://docs.aws.amazon.com/cdk/api/latest/python/aws_cdk.aws_eks.README.html

@eladb eladb reopened this Jul 26, 2020
@eladb
Copy link
Contributor

eladb commented Jul 26, 2020

Good point. The docs should be fixed. Reopening to track

@eladb eladb changed the title [AWS-EKS] cluster.add_resource got an unexpected keyword argument 'api_version' [AWS-EKS] docs issue - cluster.add_resource got an unexpected keyword argument 'api_version' Jul 26, 2020
@eladb
Copy link
Contributor

eladb commented Jul 27, 2020

The root cause is that auto-generated documentation in Python does not take into account the fact that manifest is a variable arguments array and hoists the typescript struct to keyword arguments.

Related: #826

Transferring to the jsii repo for follow up.

@eladb eladb transferred this issue from aws/aws-cdk Jul 27, 2020
@eladb eladb changed the title [AWS-EKS] docs issue - cluster.add_resource got an unexpected keyword argument 'api_version' rossetta issue: varargs are ignored when hoisting keyword arguments Jul 27, 2020
@eladb eladb removed their assignment Jul 27, 2020
@thomaspoignant
Copy link
Author

Thanks for the reassignment @eladb.

@thomaspoignant
Copy link
Author

For information, if someone has the same problems, the correct syntax is bellow.

api_version should be apiVersion,
You should use camel case not snake case.

cluster.add_resource('MyResource', {
    "apiVersion": "v1",
    "kind": "Pod",
    "metadata": {"name": "mypod"},
    "spec": {
        "containers": [{
            "name": "hello",
            "image": "paulbouwer/hello-kubernetes:1.5",
            "ports": [{"container_port": 8080}]
        }]
    }
})

@SomayaB SomayaB added bug This issue is a bug. documentation This is a problem with documentation. language/python Related to Python bindings labels Jul 27, 2020
@RomainMuller
Copy link
Contributor

The original type signature is the following:

addResource(id: string, ...manifest: any[]): KubernetesResource

This particular example was generated without compilation, so it did not have access to the type information during transliteration. The first thing we should attempt is trying to make the original version compilable and see whether this fixes it.

RomainMuller added a commit that referenced this issue Jul 29, 2020
When processing function calls, the signature of the function was not
considered when it was available, and an object literal used as the
final argument of a variadic call would be rendered as keyword arguments
when this is actually not a correct way to render variadic values.

This change attempts to read the resolved signature of the function
beging called, in order to correctly render variadic values.

Fixes #1821
@RomainMuller
Copy link
Contributor

Turns out even with type information the "bad" code would be generated in this case... I've submitted a PR to address that.

@mergify mergify bot closed this as completed in #1832 Jul 29, 2020
mergify bot pushed a commit that referenced this issue Jul 29, 2020
When processing function calls, the signature of the function was not
considered when it was available, and an object literal used as the
final argument of a variadic call would be rendered as keyword arguments
when this is actually not a correct way to render variadic values.

This change attempts to read the resolved signature of the function
beging called, in order to correctly render variadic values.

Fixes #1821



---

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
@RomainMuller
Copy link
Contributor

RomainMuller commented Aug 3, 2020

Note - in order for the documentation to actually be fixed, the example code must be made compilable (otherwise the type information needed to determine the call is variadic will not be available).

This can be achieved by dropping a rosetta/default.ts-fixture in the package which provides the necessary wrapping code so jsii-rosetta can have type info.

The necessary fix will be available in jsii's next release (1.10.0), but the fixture can be introduced before then - the translated example will simply keep being broken until CDK uses the fixed release.

@RomainMuller RomainMuller reopened this Aug 3, 2020
@RomainMuller RomainMuller added the effort/small Small work item – less than a day of effort label Aug 5, 2020
@RomainMuller
Copy link
Contributor

RomainMuller commented Aug 26, 2020

This is actually fixed in aws/aws-cdk@3c116f3 where I inadvertently committed a fixture...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort language/python Related to Python bindings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants