5758 for Node JS #8047
5758 for Node JS #8047
Conversation
t0yv0
changed the title
|
Diff for pulumi-azuread with merge commit d842d4b |
|
Diff for pulumi-random with merge commit d842d4b |
|
Diff for pulumi-random with merge commit adc68fb |
|
Diff for pulumi-azuread with merge commit adc68fb |
|
Diff for pulumi-kubernetes with merge commit d842d4b |
|
Diff for pulumi-gcp with merge commit d842d4b |
|
Diff for pulumi-kubernetes with merge commit adc68fb |
|
Diff for pulumi-gcp with merge commit adc68fb |
|
Diff for pulumi-azuread with merge commit aed6f5b |
|
Diff for pulumi-random with merge commit aed6f5b |
|
Diff for pulumi-kubernetes with merge commit aed6f5b |
|
Diff for pulumi-gcp with merge commit aed6f5b |
|
Diff for pulumi-azure with merge commit adc68fb |
|
Diff for pulumi-aws with merge commit adc68fb |
|
Diff for pulumi-aws with merge commit d842d4b |
|
Diff for pulumi-azure with merge commit d842d4b |
|
Diff for pulumi-aws with merge commit aed6f5b |
|
Unfortunately, Azure Native is falling further and further behind on pulumi/pulumi freshness due to the Go SDK size limitation. Does the check failure looks related to your change? I don't see it yet. |
|
Diff for pulumi-azure with merge commit aed6f5b |
NO, master branch is still affected by this check failure. This particular PR would only add to the Node SDK size, not Go. That being said I'm interested in what can we do to help azure-native catch up. |
|
@t0yv0 I'm merging the fix for azure-native today as soon as tests pass. |
|
But TBH I don't have a solution for the size issues with azure-native yet. Sorry, lots of other things with overlapping priorities. The fix is only for the enum naming collisions that'll fix the downstream tests here. |
|
Curious, does your fix result in reducing Go codegen size for azure-native also? A few semantic fixes were on my radar, like #7985 but the problem with generating too much code would remain after that. |
|
#7835 was one idea we could still pursue to fix that. |
| // limitations under the License. | ||
|
|
||
| import 'mocha'; | ||
| import * as assert from 'assert'; |
There was a problem hiding this comment.
Nit: I don't feel too strongly about this, but we generally use double quotes in our other ts code. At some point we should make sure these files are linted (and style checked + auto formatted if/when we hook that up).
|
Diff for pulumi-aws with merge commit e7a09ae |
|
Diff for pulumi-azure with merge commit e7a09ae |
|
Diff for pulumi-azure-native with merge commit e7a09ae |
|
I'm having trouble reviewing this due to the volume of added baselines. Would it be possible to restructure the commits a bit s.t. the codegen changes are in a single commit? |
|
Yes, definitely, let me do that. |
t0yv0
force-pushed
the
t0yv0/5758-node-rebased
branch
from
54ccf01
to
de6ceb6
Compare
pkg/codegen/nodejs/gen.go
Outdated
| @@ -622,7 +629,7 @@ func (mod *modContext) genResource(w io.Writer, r *schema.Resource) error { | |||
| allOptionalInputs = true | |||
| } | |||
|
|
|||
| // Write out callable constructor: We only emit a single public constructor, even though we use a private signature | |||
| // Writef out callable constructor: We only emit a single public constructor, even though we use a private signature | |||
There was a problem hiding this comment.
Typo, pressed a wrong Emacs chord here.
|
@pgavlin PTAL, two-commit version, first commit has the essential changes, and the second one has the combination of new tests providing coverage and the less meaningful noise of propagating changes to all langs. |
|
Diff for pulumi-random with merge commit 22947ad |
|
Diff for pulumi-azuread with merge commit 22947ad |
|
Diff for pulumi-gcp with merge commit 22947ad |
|
Diff for pulumi-kubernetes with merge commit 22947ad |
|
Diff for pulumi-azure with merge commit 22947ad |
|
Diff for pulumi-aws with merge commit 22947ad |
|
Diff for pulumi-azure-native with merge commit 22947ad |
|
Diff for pulumi-random with merge commit 7a1cbac |
|
Diff for pulumi-azuread with merge commit 7a1cbac |
|
Diff for pulumi-kubernetes with merge commit 7a1cbac |
|
Diff for pulumi-gcp with merge commit 7a1cbac |
|
Diff for pulumi-aws with merge commit 7a1cbac |
|
Diff for pulumi-azure with merge commit 7a1cbac |
|
Diff for pulumi-azure-native with merge commit 7a1cbac |
Description
Fixes #7944
Part of #5758 for Node JS.
This brings Node JS to parity with Go and Python. .NET is pending.
@mikhailshilkov adding you to see if there are blockers to merging this we need to address first. This is a change in codegen and it increases the size of the generated packages. Currently PR checks on azure-native show up as failing which is slightly concerning to me. In the beginning of developing this feature I manually rebuilt azure-native and found a TypeScript compilation issue. I've cut out a part of the azure-native schema and added it to the test suite, so I do not believe the particular issues is happening again, but this is good to discuss. Should I go ahead and try to manually built azure-native with this change prior to merging?
Thank you.
Checklist