-
Notifications
You must be signed in to change notification settings - Fork 50
Mark up docs schema, and add language specific filter #3188
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3188 +/- ##
========================================
Coverage 68.78% 68.78%
========================================
Files 336 336
Lines 43868 43995 +127
========================================
+ Hits 30174 30264 +90
- Misses 11996 12025 +29
- Partials 1698 1706 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed much of the code yet, but I'm curious what you think about the prior discussions on this issue? Specifically related to pulumi/pulumi#16099 and the work started on pulumi/pulumi#18949.
From what I can follow it seems like the plan was to not require tfgen
at all. It seems like it wouldn't be too hard to at a later point change the span generation to the tag/ref
generation, but I'm also not sure how close we are to having 16099 finished. It seems like we could either:
- Finish 16099 and then implement the
tag/ref
generation here - Merge this PR, then finish 16099, then come back and modify this generation.
I think it all depends on how close we are to merging 18949 / fixing 16099. If we are pretty close then I think we should try to push that through.
Thoughts?
pkg/tfgen/docs.go
Outdated
} | ||
|
||
// Build our span | ||
// <span pulumi-lang-nodejs="firstProperty" pulumi-lang-go="FirstProperty" ...>firstProperty</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the linked issues it looks like there was some agreement on a different format
<pulumi ref="#/resources/aws:s3:Bucket/properties/bucketName" />
thank you for the review! So there's a couple of things here.
Right! There's two separate
This PR modifies 2) to get us closer to the goal of not using tfgen at all. TL;DR: I think this PR is a stepping stone to the goals; moreover I don't think it makes any irreversible decisions that would stand in the way of implementing pulumi/pulumi#16099. |
017c089
to
a59efa4
Compare
{ | ||
"name": "random", | ||
"description": "A Pulumi package to safely use randomness in Pulumi programs.", | ||
"keywords": [ | ||
"pulumi", | ||
"random" | ||
], | ||
"homepage": "https://pulumi.io", | ||
"license": "Apache-2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love all the testing you added! Just wanted to call out how great that is to have some more unit test coverage!
db0b271
to
dc40b9c
Compare
|
||
// First, generate the schema file that the NodeJS generator expects | ||
// Use a separate in-memory filesystem for schema generation to avoid conflicts | ||
schemaRoot := afero.NewMemMapFs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
// Use `ec2.Instance` format for Go and Python | ||
goAndPyFormat := open + modname + resname.String() + close | ||
// Use `aws.ec2.Instance` format for all other languages | ||
allOtherLangs := open + c.pkg.String() + "." + modname + resname.String() + close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the text we get is already appropriately snake/pascal-cased, we don't need to handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! The resource name is already Pulumified.
pkg/tfgen/docs_test.go
Outdated
assert.Equalf(t, text == "", elided, | ||
"We should only see an empty result for non-empty inputs if we have elided text") | ||
for _, tc := range testCases { | ||
tc := tc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since Go 1.22.
…n with language tags.
…er now. add regex filters. Thought - could this be done better? UNCLEAR
…its to inline autogold strings
dc40b9c
to
ab52e74
Compare
ab52e74
to
a833c6b
Compare
f82e085
to
df8cfcf
Compare
7d9a18c
to
e10f8b9
Compare
e10f8b9
to
9fc1766
Compare
This pull request does several things:
fixUpPropertyReference
function to mark up any such reference with a tag that contains language-specific keys and values so a language-specific filter function can identify which value should be used for the language selected.The span looks as follows in the schema:
\u003cspan pulumi-lang-nodejs=\"
minUpper\" pulumi-lang-dotnet=\"
MinUpper\" pulumi-lang-go=\"
minUpper\" pulumi-lang-python=\"
min_upper\" pulumi-lang-yaml=\"
minUpper\" pulumi-lang-java=\"
minUpper\"\u003e
min_upper\u003c/span\u003e
fixUpPropertyReference
where Dotnet was generating some property names insnake_case
(TODO: proper capitalization for these property names is still missing) - see examplefixUpPropertyReference
; adds tests for the filter function.In this pulumi-random PR, you can see the changes to the SDK are minimal (the azure -> azurerm is unrelated; the Java changes are due to #3184 and haven't been rolled out to providers.
Additional benefits:
This pull request does not address the following:
I ran this against the AWS provider as well. The SDK inline documentation has a few extra empty section headers with this change, because we're not running the
description
field through the docs generator twice. I think this is the correct bargain to make - after this change, we can fix the docs rendering in one place, and have the schema be the source of truth for everything.Fixes #1918.