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

[codegen/go] Emit pulumiplugin.json as part of codegen #8530

Merged
merged 4 commits into from
Dec 6, 2021

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Dec 2, 2021

Description

Fixes #8529

Note:

  • The first commit includes the code changes.
  • The second commit updates the tests.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

Test changes will follow in a later commit.
@iwahbe iwahbe self-assigned this Dec 2, 2021
@iwahbe iwahbe requested review from justinvp and a team December 2, 2021 01:43
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azuread with merge commit 4770251

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-random with merge commit 4770251

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azuread with merge commit c93c83e

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-random with merge commit c93c83e

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-gcp with merge commit 4770251

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-kubernetes with merge commit 4770251

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-kubernetes with merge commit c93c83e

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-gcp with merge commit c93c83e

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure with merge commit 4770251

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure with merge commit c93c83e

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-aws with merge commit c93c83e

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-aws with merge commit 4770251

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #8530 (4d16c6b) into master (a79a759) will increase coverage by 0.03%.
The diff coverage is 74.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8530      +/-   ##
==========================================
+ Coverage   58.61%   58.64%   +0.03%     
==========================================
  Files         634      634              
  Lines       96844    96877      +33     
  Branches     1378     1378              
==========================================
+ Hits        56762    56815      +53     
+ Misses      36819    36796      -23     
- Partials     3263     3266       +3     
Impacted Files Coverage Δ
sdk/go/pulumi-language-go/main.go 23.91% <72.22%> (+1.44%) ⬆️
pkg/codegen/go/gen.go 88.43% <75.00%> (-0.06%) ⬇️
sdk/go/common/resource/plugin/plugin.go 66.66% <100.00%> (ø)
pkg/codegen/hcl2/model/type_eventuals.go 92.57% <0.00%> (-0.44%) ⬇️
sdk/nodejs/automation/localWorkspace.ts 73.64% <0.00%> (-0.39%) ⬇️
pkg/codegen/python/importer.go 30.00% <0.00%> (ø)
pkg/codegen/internal/test/sdk_driver.go 82.14% <0.00%> (ø)
pkg/codegen/dotnet/gen.go 82.24% <0.00%> (+0.12%) ⬆️
pkg/codegen/hcl2/model/type_object.go 89.72% <0.00%> (+0.54%) ⬆️
pkg/codegen/python/gen.go 82.81% <0.00%> (+0.63%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a79a759...4d16c6b. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure-native with merge commit 4770251

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure-native with merge commit c93c83e

if err != nil {
return nil, fmt.Errorf("Failed to format pulumiplugin.json: %w", err)
}
files[path.Join(pathPrefix, "pulumiplugin.json")] = pulumiPlugin
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how we want to handle generating this file for Go. With the change to the Go language host, it's going to be looking for the pulumiplugin.json file next to go.mod:

path := filepath.Join(m.Dir, "pulumiplugin.json")

For example, for pulumi-random, the lang host will be looking for the file in:

./sdk/pulumiplugin.json

but the codegen is currently generating it here:

./sdk/go/random/pulumiplugin.json

Not sure what we want to do about this. Some ideas (I don't love these):

  1. Don't generate the file for Go. This would be a little strange since we're now generating the file for Python and .NET. But it does provide the most flexibility for Go. For most providers, you could add the file manually next to the go.mod. For the larger providers (i.e. Azure Native) where we're considering breaking it up into multiple modules, we can manually place a pulumiplugin.json next to the go.mod for all of those modules.

  2. Update the language host to probe for the file in more places. I'm not sure if there's a good way to do this. Our typical convention is to have sdk then go then the name of the package like random. So I guess we could have the language host try to look for the pulumiplugin.json file there. But I'm not sure that convention is always going to be followed, especially considering we may break up Azure Native into multiple modules.

Any other ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! To make matters even more complicated, sdk/go/random is a configurable option, it's not hardcoded. The current lookup method requires that pulumiplugin.json is in the same folder as go.mod, which makes sense to me. Except that it would mean putting another go specific file in the sdk folder.

Let me think for a bit.

Copy link
Member Author

@iwahbe iwahbe Dec 2, 2021

Choose a reason for hiding this comment

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

After discussing offline, I intend to do the following

  • Continue to generate pulumiplugin.json in m.Dir/go/packagemane by default.
  • Unless rootPackageName is set. Then I will generate pulumiplugin.json in the top level directory. It will be the responsibility of the codegen user to move it next to go.mod.
  • Change pulumiplugin.json lookup to probe
    • m.Dir
    • m.Dir/go
    • m.Dir/go/${package name}

@iwahbe iwahbe requested a review from justinvp December 2, 2021 20:46
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-random with merge commit 18b1e45

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azuread with merge commit 18b1e45

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-kubernetes with merge commit 18b1e45

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-gcp with merge commit 18b1e45

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure with merge commit 18b1e45

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-aws with merge commit 18b1e45

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure-native with merge commit 18b1e45

@iwahbe iwahbe requested a review from a team December 3, 2021 17:17
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM

@iwahbe iwahbe merged commit c02bff9 into master Dec 6, 2021
@pulumi-bot pulumi-bot deleted the iwahbe/8529/go-emit-pulumiplugin-json branch December 6, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[codegen/go] Emit pulumiplugin.json as part of codegen.
2 participants