pulumi / pulumi-azure-native

Azure Native Provider
Apache License 2.0
127 stars 34 forks source link

Highlight that Python resource_name args are appended with `_` #2423

Open logileifs opened 2 years ago

logileifs commented 2 years ago

When creating a resource with a resource argument named resource_name, it is renamed to resource_name_ - with an extra _ suffix when using the overload of the constuctor which expands the args directly into the constructor.

We should add a note to the Python inputs args documentation indicating when this field might have a differnt name.

Original Issue

What happened?

Pulumi appends a random suffix to the name.

Steps to reproduce

resource_group = resources.ResourceGroup(
    "azure-native-py-aks",
    location='northeurope'
)

ssh_key = tls.PrivateKey("ssh-key", algorithm="RSA", rsa_bits=4096)

managed_cluster = containerservice.ManagedCluster(
    resource_name='my-new-aks-cluster',
    resource_group_name=resource_group.name,
    agent_pool_profiles=[{
        "count": 1,
        "max_pods": 110,
        "mode": "System",
        "name": "agentpool",
        "node_labels": {},
        "os_disk_size_gb": 128,
        "os_type": "Linux",
        "type": "VirtualMachineScaleSets",
        "vm_size": "Standard_B2s",
    }],
    enable_rbac=True,
    kubernetes_version="1.22.6",
    linux_profile={
        "admin_username": "testuser",
        "ssh": {
            "public_keys": [{
                "key_data": ssh_key.public_key_openssh,
            }],
        },
    },
    dns_prefix=resource_group.name,
    node_resource_group=f"MC_azure-native-go_my-aks-cluster_northeurope",
    service_principal_profile={
        "client_id": config.require('client_id'),
        "secret": config.require('client_secret')
    }
)

Expected Behavior

Cluster should be created with name my-new-aks-cluster.

Actual Behavior

Cluster is created with name like my-new-aks-cluster80aa82f8.

Output of pulumi about

CLI
Version 3.37.2 Go Version go1.17.12 Go Compiler gc

Plugins NAME VERSION azure-native 1.78.0 azuread 4.3.0 python unknown random 4.8.2 tls 4.6.1

Host
OS ubuntu Version 20.04 Arch x86_64

This project is written in python: executable='/home/logi/.virtualenvs/aks/bin/python3' version='3.9.14'

Current Stack: test

TYPE URN pulumi:pulumi:Stack urn:pulumi:test::aks::pulumi:pulumi:Stack::aks-test pulumi:providers:tls urn:pulumi:test::aks::pulumi:providers:tls::default_4_6_1 pulumi:providers:azure-native urn:pulumi:test::aks::pulumi:providers:azure-native::default_1_78_0 azure-native:resources:ResourceGroup urn:pulumi:test::aks::azure-native:resources:ResourceGroup::azure-native-py-aks tls:index/privateKey:PrivateKey urn:pulumi:test::aks::tls:index/privateKey:PrivateKey::ssh-key azure-native:containerservice:ManagedCluster urn:pulumi:test::aks::azure-native:containerservice:ManagedCluster::skelfir-test-cluster pulumi:providers:azure-native urn:pulumi:test::aks::pulumi:providers:azure-native::default

Found no pending operations associated with test

Backend
Name ux580ge URL file://~ User logi Organizations

Dependencies: NAME VERSION pip 22.2.2 pulumi-azure-native 1.78.0 pulumi-azuread 4.3.0 pulumi-random 4.8.2 pulumi-tls 4.6.1 setuptools 65.1.1 wheel 0.37.1

Pulumi locates its logs in /tmp by default

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

logileifs commented 2 years ago

After filing this issue I took a look at the code in this repository and noticed that the cluster resource also accepts a parameter called resource_name_. If I use that one the cluster gets created with the resource_name_ name. That is however very confusing and I couldn't see it documented anywhere, is that by design?

danielrbradley commented 2 years ago

Yes, you're correct that you can override this behaviour using resource_name. Autonaming is a feature global to all Pulumi providers - more detail can be found in the resource naming page in the documentation.

logileifs commented 2 years ago

I feel like you didn't really read the issue or my comment. I have read the documentation about the resource names. I have taken a look at examples and read people talk about overriding the name with resource_name I have however never seen documented anywhere the usage of a parameter called resource_name_. That is an undocumented feature as far as I can tell and no way to figure that out unless you dive into the code which is not a very nice user experience.

danielrbradley commented 2 years ago

Appologies if I misunderstood what you were attempting to communicate. I'm not certain what the reason for the trailing underscore is, but would assume it might be related to resource_name being a reserved keyword.

logileifs commented 2 years ago

No biggie, perhaps I wasn't making myself clear enough. I don't know much about pulumi internals but I assume since the ManagedCluster constructor is already accepting a resource_name parameter it could just as well use that one instead of the extra resource_name_ to actually set the name.

danielrbradley commented 2 years ago

Ok, having looked into this a little more, the naming of the input is because of how the Python SDK flattens the resource args into the constructor arguments. The first constructor argument is resource_name and therefore a resource arg of resource_name is suffixed with _ to avoid conflicts. This is reflected correct in the example python code but is not called out in the field documentation.

Will transfer this to the docs as a python-specific docs improvement.

danielrbradley commented 2 years ago

A complicating matter comes from the fact that there's another overload of the constructor which takes a ManagedClusterArgs object, instead of the expanded set of named arguments. In this case, the resource argument resource_name remains with its original name without the suffix.

danielrbradley commented 1 year ago

Hey @susanev I notice you transferred this back out of docs. From my understanding the python naming is a general problem rather than specific to any individual provider so needs to be solved at the point where we generate the documentaion from the schema which was why I transferred this over to docs. Is docs not the right place for this to be filled?