-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Resource docs: emit supporting types beyond the 200 limit #16185
Conversation
Changelog[uncommitted] (2024-05-13)Features
|
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.
LGTM
// types or add a limit. The default is including all types up to 1000. | ||
maxNestedTypes := 1000 | ||
switch mod.pkg.Name() { | ||
case "aws", "aws-native": |
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.
This makes me wonder if we could potentially make this configurable at the provider/schema level instead of hardcoding numbers for different packages? I don't know how exactly this codegen'ing works, but from an outside perspective it would make more sense if the provider had control over this, rather than having codegen hardcode provider names here.
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.
Yeh I suppose this could go in the schema Meta block?
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 had this idea too, but I would argue that this is not actually a provider's concern. It's a peculiarity of the way we display docs and a limitation of our current docs toolchain. I'm not sure I like exposing this low-value setting in the schema...
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.
It doesn't feel like codegen's concern to deal with specific packages either :-) Unless we're planning to fix this peculiarity at some point, I guess we'll have to live with it somehow, and having to add providers here whenever we need a different number feels not so great. I'd be happy with having a default of 1000 here, and just have it be an optional setting in the schema.
That said if that's too much work and we just need to fix this for now I'm not going to oppose this change too strongly, but I don't think it's the right way to go longer term.
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.
Is there a way to apply the lower limit to all pulumi 1st party packages and keep it high for all others?
codegen's concern
I'd love to move docsgen out of pu/pu, to be honest. It feels a bit misplaced overall.
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 that I know of unfortunately.
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 had this idea too, but I would argue that this is not actually a provider's concern. It's a peculiarity of the way we display docs and a limitation of our current docs toolchain.
Fair, I'm happy with this as is.
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 that I know of unfortunately
The schema has a repo URL, I guess I could thread it into the PackageReference and then apply to schemas pointing to our GH org. Would this be better?
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.
Maybe? But as others are happy with this I think we can also go ahead with it as it is and the revisit in the future if we end up needing to hardcode more providers here.
### Features - [docs] Resource docs: bump the number of displayed supporting types from 200 to 1000 by default [#16185](#16185) - [sdk/go] Prefer pluginDownloadURLOverrides over PluginDownloadURL specified in the package [#16186](#16186) ### Bug Fixes - [engine] Fix panic when using `pulumi up --refresh --continue-on-error` [#16184](#16184)
Description
Originally motivated by uncontrolled pseudo-recurcive type expansion in AWS WafV2, we've limited the number of types that we should in the docs to 200: #12070
Our large customer that publishes their own packages and docs came back to us and said they have legitimate use cases with more than 200 types: #15507
I've grabbed stats about our current packages and still found a few offenders:
Therefore, I'm not entirely removing the limit in this PR, but
a) bumping the default to 1000
b) applying 200 to the known offenders only
I don't love it's hard coded, but I haven't found a place to add simple configuration nob. Anyway, it's slightly less hard-coded than it used to be.
Fixes #15507
Checklist
make tidy
to update any new dependenciesmake lint
to verify my code passes the lint checkgofumpt
make changelog
and committed thechangelog/pending/<file>
documenting my change