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

Resource docs: emit supporting types beyond the 200 limit #16185

Merged
merged 2 commits into from
May 13, 2024

Conversation

mikhailshilkov
Copy link
Member

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:

"aws:lex/v2modelsIntent:V2modelsIntent" 920
"aws:wafv2/ruleGroup:RuleGroup" 310
"aws:wafv2/webAcl:WebAcl" 523
"azure-native:datafactory:Dataset" 256
"azure-native:datafactory:LinkedService" 299
"azure-native:datafactory:Pipeline" 618
"azure-native:datamigration:ServiceTask" 291
"azure-native:datamigration:Task" 291
"aws-native:quicksight:Analysis" 589
"aws-native:quicksight:Dashboard" 606
"aws-native:quicksight:Template" 590

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

  • 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
    • Existing docs gen tests cover that I haven't broken anything
    • I re-generated the AWS docs and they had no changes
  • 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

@pulumi-bot
Copy link
Contributor

pulumi-bot commented May 13, 2024

Changelog

[uncommitted] (2024-05-13)

Features

  • [docs] Resource docs: bump the number of displayed supporting types from 200 to 1000 by default
    #16185

@mikhailshilkov mikhailshilkov marked this pull request as ready for review May 13, 2024 10:43
@mikhailshilkov mikhailshilkov requested a review from a team as a code owner May 13, 2024 10:43
Copy link
Contributor

@julienp julienp left a 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":
Copy link
Collaborator

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.

Copy link
Member

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?

Copy link
Member Author

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...

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Collaborator

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.

@mikhailshilkov mikhailshilkov added this pull request to the merge queue May 13, 2024
Merged via the queue into master with commit 9388c54 May 13, 2024
55 checks passed
@mikhailshilkov mikhailshilkov deleted the mikhailshilkov/docs-types-beyond-200 branch May 13, 2024 16:21
@justinvp justinvp mentioned this pull request May 14, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 15, 2024
### 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)
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.

Add ability to configure emitting large supporting types beyond the 200 limit
5 participants