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

[docs] Fix generating constructor examples for resources that have numeric enums as input #16223

Merged
merged 1 commit into from
May 30, 2024

Conversation

Zaid-Ajaj
Copy link
Contributor

Description

Fixes #16191

The original issue is that the intermediate PCL we generate used enum names instead of enum values for numeric enum inputs. This PR changes it so that the PCL program now uses the first numeric value for the first enum case then subsequently fixing downstream program-gen bugs that didn't know how to handle numeric values as inputs for enums.

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@Zaid-Ajaj Zaid-Ajaj added area/docs Improvements or additions to documentation area/codegen SDK-gen, program-gen, convert labels May 17, 2024
@Zaid-Ajaj Zaid-Ajaj requested a review from a team as a code owner May 17, 2024 22:41
@pulumi-bot
Copy link
Contributor

pulumi-bot commented May 17, 2024

Changelog

[uncommitted] (2024-05-29)

Bug Fixes

  • [docs] Fix generating constructor examples for resources that have numeric enums as input
    #16223

Comment on lines 65 to 78
"fooNumericEnum": {
TypeSpec: schema.TypeSpec{
Ref: "#/types/test:index:ExampleEnum",
},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add a test case for non-numeric enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

from.Type(), from, to))
} else {
// for numeric enums, we new up the type that is generated for that enum
convertFn = fmt.Sprintf("new %s.%s", pkg, name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test that demonstrates this? Do we really use new?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the generated and their was a constructor that accepts double but now I checked again and that constructor is private 😅

private TeamStackPermissionScope(double value)

there is actually no way to construct the enum from a number (it's generated as a struct). I changed the previous behaviour because it was panicking too early and unnecessarily so I though I would just fix it but this won't do. Will bring back the previous behaviour without redundantly panicking. Also we could change sdk-gen in dotnet to make the constructor public or add an implicit cast operator but that is for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old behaviour is back without panicking too early ✅ constructing a numeric enum from a number value will be handled in a separate PR

@Zaid-Ajaj Zaid-Ajaj force-pushed the zaid/fix-numeric-enums-program-gen branch from c8fbb92 to a31a6db Compare May 18, 2024 13:38
@Zaid-Ajaj Zaid-Ajaj requested a review from justinvp May 18, 2024 13:40
@Zaid-Ajaj Zaid-Ajaj force-pushed the zaid/fix-numeric-enums-program-gen branch from 5bc364b to 0c9178d Compare May 18, 2024 14:10
Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks ok, odd that java and yaml are using the numeric values rather than enums though? Is that because they need fixes in their code generators?

@Zaid-Ajaj
Copy link
Contributor Author

Is that because they need fixes in their code generators?

@Frassle that's right

@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@Zaid-Ajaj Zaid-Ajaj force-pushed the zaid/fix-numeric-enums-program-gen branch from 090b6a4 to b075f9d Compare May 29, 2024 22:05
@Zaid-Ajaj Zaid-Ajaj enabled auto-merge May 29, 2024 22:06
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2024
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 30, 2024
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 30, 2024
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue May 30, 2024
Merged via the queue into master with commit de54e1a May 30, 2024
57 of 63 checks passed
@Zaid-Ajaj Zaid-Ajaj deleted the zaid/fix-numeric-enums-program-gen branch May 30, 2024 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen SDK-gen, program-gen, convert area/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing constructor syntax examples for the Pulumi service provider
4 participants