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.

Sign up for GitHub

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

Jump to bottom

[sdk/go] pulumi.IDOutput needs ToInt*Output methods #11750

Open
daenney opened this issue Dec 30, 2022 · 2 comments
Open

[sdk/go] pulumi.IDOutput needs ToInt*Output methods #11750

daenney opened this issue Dec 30, 2022 · 2 comments
Labels
area/sdks Pulumi language SDKs kind/enhancement Improvements or new features

Comments

@daenney
Copy link

daenney commented Dec 30, 2022

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

This reflects the issue raised on: pulumi/pulumi-hcloud#52.

In a number of providers, including Hetzner and Digital Ocean, the ID of a resource returned by their API is a number and is expected to be passed in as an int in many parts of the API, not a string or stringified int. This results in having to write really awkward code in Go like:

hcloud.NewServer(ctx, name, &hcloud.ServerArgs{
...
		PublicNets: hcloud.ServerPublicNetArray{
			&hcloud.ServerPublicNetArgs{
				Ipv4Enabled: pulumi.Bool(true),
				Ipv6Enabled: pulumi.Bool(true),
				Ipv4:        server.ipv4.ID().ToStringOutput().ApplyT(IDtoIntPtr).(pulumi.IntPtrOutput),
				Ipv6:        server.ipv6.ID().ToStringOutput().ApplyT(IDtoIntPtr).(pulumi.IntPtrOutput),
			},
		},
	},

With IDtoIntPtr being:

func IDtoIntPtr(val string) (*int, error) {
	i, err := strconv.Atoi(val)
	return &i, err
}

The fact that we don't have a ToInt(Ptr)Output on the IDOutput makes this code really verbose. When you initially run into this issue it's also really hard to figure out how to get around it as you need to realise you need to chain ToStringOutput with ApplyT.

There might be other ways to solve this, but so far the ToStringOutput().ApplyT() chain seems the only solution people have come up with.
@daenney daenney added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Dec 30, 2022
@daenney
Copy link
Author

daenney commented Dec 30, 2022 •
edited

One thing I'm also wondering, hcloud.ServerPublicNetArgs.Ipv4 expects a pulumi.IntPtrOutput but it seems to me it should directly accept a pulumi.ID(Ptr)Input instead? I'm guessing this has something to do with how the Terraform bridge fits in since pulumi-hcloud wraps the Hetzner Cloud terraform provider. Pulumi would still need to deal with the fact that the Hetzner provider (and Hetzner's API) expects that ID to be an integer.

There are a few cases in the Hetzner Cloud API where this doesn't work as a resource accepts both the integer ID or the name of the created resource. I'm not sure what to do about those.

@kpitzen
Copy link
Contributor

kpitzen commented Jan 3, 2023

Hi @daenney - thank you for raising this issue! We're in the process of adding similar helper methods like this across our Input/Output surface area, so we definitely appreciate the feedback. I'll be sure this gets added to the list. Thanks again!

@kpitzen kpitzen added area/sdks Pulumi language SDKs and removed needs-triage Needs attention from the triage team labels Jan 3, 2023
@Frassle Frassle mentioned this issue Jan 5, 2023
Closed
@abhinav abhinav mentioned this issue Jan 18, 2023
Merged
abhinav added a commit that referenced this issue Jan 18, 2023
@abhinav
ApplyT: Coerce new type wrappers
d0aac86 
Adds support to ApplyT to automatically coerce new type wrappers
when calling the provided functions.

With this change, the following is valid:

    var idout pu.IDOutput = ...
    idout.ApplyT(func(id string) string {
        // ...
    })

Note that this does not use `To{Type}Output` methods at this time.
We will likely want to add that in the future given #11750.
We can do that in a backwards compatible way.

This coerces only those values that are defined
with language-level type wrappers in the following form:

    type ID string

This *does not* coerce types with different undedrlying representations.
Specifically, the following conversons are not supported
even though they're supported by Go using the `T(v)` syntax.

    string => int
    string => []byte
    []byte => string

The choice to convert between these types
should be made explicitly by the user
so that they get the semantics they want.

Resolves #11784
abhinav added a commit that referenced this issue Jan 18, 2023
@abhinav
ApplyT: Coerce new type wrappers
5fb3582 
Adds support to ApplyT to automatically coerce new type wrappers
when calling the provided functions.

With this change, the following is valid:

    var idout pu.IDOutput = ...
    idout.ApplyT(func(id string) string {
        // ...
    })

Note that this does not use `To{Type}Output` methods at this time.
We will likely want to add that in the future given #11750.
We can do that in a backwards compatible way.

This coerces only those values that are defined
with language-level type wrappers in the following form:

    type ID string

This *does not* coerce types with different undedrlying representations.
Specifically, the following conversons are not supported
even though they're supported by Go using the `T(v)` syntax.

    string => int
    string => []byte
    []byte => string

The choice to convert between these types
should be made explicitly by the user
so that they get the semantics they want.

Resolves #11784
abhinav added a commit that referenced this issue Jan 19, 2023
@abhinav
ApplyT: Coerce new type wrappers
772a5fd 
Adds support to ApplyT to automatically coerce new type wrappers
when calling the provided functions.

With this change, the following is valid:

    var idout pu.IDOutput = ...
    idout.ApplyT(func(id string) string {
        // ...
    })

Note that this does not use `To{Type}Output` methods at this time.
We will likely want to add that in the future given #11750.
We can do that in a backwards compatible way.

This coerces only those values that are defined
with language-level type wrappers in the following form:

    type ID string

This *does not* coerce types with different undedrlying representations.
Specifically, the following conversons are not supported
even though they're supported by Go using the `T(v)` syntax.

    string => int
    string => []byte
    []byte => string

The choice to convert between these types
should be made explicitly by the user
so that they get the semantics they want.

Resolves #11784
bors bot added a commit that referenced this issue Jan 19, 2023
@bors @aq17
@sean1588 @RobbieMcKinstry @abhinav
Merge #11803 #11893 #11903 #11922
2ef87d8 
11803: Display text-based diff if yaml/json diff is semantically equal r=aq17 a=aq17

Fixes #11799 

#9380 and #9484 introduced changes that render JSON/YAML parsable strings as objects in diffs – therefore, examples such as`"foo:\n  bar: {}\n  baz: {}\n"` and `"#foo\nfoo:\n  bar: {}\n  baz: {}\n"` are recognized as equal.

This change causes the display to fall back to showing the text-based diff if the JSON/YAML rendering does not indicate any diff.
It also prints all object keys even if the value is empty, i.e. `bar: {}` or `bar: ""` will be printed instead of `bar` being omitted.

11893: update package details anchors r=sean1588 a=sean1588

fixes: pulumi/registry#1819

OK taking this for round 2. The first [PR](#11861) to make this change I ended up reverting because there were some pages (the function pages and module pages) still being generated with empty href tags. 

I have addressed this and updated the change to define the DisplayName and RepositoryName props on the `packageDetails` struct for the individual module pages and function pages. This updates the repository anchor in the Package Details section of the api docs pages. I updated it to display `<package name> repository` for the anchor text in place of the url that was previously shown there. 

![image](https://user-images.githubusercontent.com/16751381/213036900-2938a9ad-1992-4170-9b83-0fbbcef409a0.png)

So now this will show the following on the following pages:
1. the root index package pages
2. the module pages
3. the resource pages
4. the function pages

11903: ApplyT: Coerce new type wrappers r=abhinav a=abhinav

Adds support to ApplyT to automatically coerce new type wrappers
when calling the provided functions.

With this change, the following is valid:

    var idout pu.IDOutput = ...
    idout.ApplyT(func(id string) string {
        // ...
    })

Note that this does not use `To{Type}Output` methods at this time.
We will likely want to add that in the future given #11750.
We can do that in a backwards compatible way.

This coerces only those values that are defined
with language-level type wrappers in the following form:

    type ID string

This *does not* coerce types with different undedrlying representations.
Specifically, the following conversons are not supported
even though they're supported by Go using the `T(v)` syntax.

    string => int
    string => []byte
    []byte => string

The choice to convert between these types
should be made explicitly by the user
so that they get the semantics they want.

Resolves #11784


11922: ci: freeze 3.52.1 for release r=dixler a=dixler

Patch release to provide better stack traces for python #11887

Co-authored-by: aq17 <aqiu@pulumi.com>
Co-authored-by: Sean Holung <sean.holung@gmail.com>
Co-authored-by: Robbie McKinstry <robbie@pulumi.com>
Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
Co-authored-by: Kyle Dixler <kyle@pulumi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/sdks Pulumi language SDKs kind/enhancement Improvements or new features
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@daenney @kpitzen