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

feat: add pulumiverse/pulumi-fortios provider #4184

Merged

Conversation

tmeckel
Copy link
Contributor

@tmeckel tmeckel commented Mar 22, 2024

Description

Add pulumiverse/pulumi-fortios to community-packages/package-list.json

Adding a new package?

If this pull request adds a new package:

  • The package's schema URL is correct.
  • The package metadata file, if present, contains:
    • a supported category (one of Cloud, Infrastructure, Network, Database, Monitoring, or Utility).
    • a description that explains what the package does.
    • a valid logo URL that points to a PNG whose dimensions conform to the others in this repo (e.g., 100x100).
    • a version number prefixed with v that corresponds with a valid GitHub release.
  • The package repo contains an Overview doc (/docs/_index.md) that includes:
    • a brief explanation of what the package is and what it does.
    • at least one representative example in all supported languages.
    • a front-matter property for the layout set to package.
  • The package repo contains an Installation and Configuration doc (/docs/installation-configuration.md) that includes:
    • links to SDKs in all supported languages.
    • a copyable command for installing the resource plugin if necessary.
    • an example of configuring the provider with pulumi config set.
    • an example of configuring the provider with environment variables.
  • A CODEOWNER has reviewed the PR.
  • A member of the @pulumi/docs team has reviewed all documentation.

Copy link
Member

@ringods ringods left a comment

Choose a reason for hiding this comment

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

@tmeckel I reviewed the provider. Can you have a look at the two items I didn't check:

@tmeckel
Copy link
Contributor Author

tmeckel commented Mar 28, 2024

@ringods can you please have a look at the PR in the provider repo (pulumiverse/pulumi-fortios#7), if the changes to the documentation reflect your expectations.

@tmeckel tmeckel requested a review from ringods March 28, 2024 16:45
@ringods
Copy link
Member

ringods commented Apr 2, 2024

@tmeckel can you rebase your PR? Due to other merges, there are conflicts now on this package list file.

@tmeckel tmeckel force-pushed the feat/onboarding-pulumiverse-fortios branch from fd2d348 to 60257f9 Compare April 2, 2024 16:18
@tmeckel
Copy link
Contributor Author

tmeckel commented Apr 2, 2024

@tmeckel can you rebase your PR? Due to other merges, there are conflicts now on this package list file.

@ringods done 😄

@cnunciato
Copy link
Contributor

cnunciato commented Apr 2, 2024

@tmeckel When I attempt to build the docs, I get an error:

panic: fatal: An assertion has failed: duplicate file: router/bgp/_index.md

Any ideas as to why? Here's where the panic occurs: https://github.com/pulumi/pulumi/blob/e86db2e79667b5cc674a94e7165437937873b220/pkg/codegen/utilities.go#L180

@tmeckel
Copy link
Contributor Author

tmeckel commented Apr 3, 2024

@tmeckel When I attempt to build the docs, I get an error:

panic: fatal: An assertion has failed: duplicate file: router/bgp/_index.md

Any ideas as to why? Here's where the panic occurs: https://github.com/pulumi/pulumi/blob/e86db2e79667b5cc674a94e7165437937873b220/pkg/codegen/utilities.go#L180

@cnunciato The root cause of this panic has been fixed with pulumi/pulumi#15035. I think you're using registrygen but registrygen is, compared to resourcedocsgen, on a pretty dated version of the Pulumi SDK, which does not contain the fix from the mentioned PR.

Just checked that resourcedocsgen isn't in the pulumi/docs repo anymore. So basically I don't really know what tools you guys are using to create the docs. So please simply ensure the tool you used has been compiled with the PR mentioned.

cc @t0yv0

image

@cnunciato
Copy link
Contributor

@tmeckel Ah, right you are, thanks! I've updated. (And yes we pulled resourcedocsgen out of pulumi/docs -- it's now in pulumi/registry.)

Copy link
Contributor

@cnunciato cnunciato left a comment

Choose a reason for hiding this comment

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

@tmeckel It looks like the docs still contain some HCL. Can you run through these and make sure they're all correct?

image

Copy link
Contributor

@cnunciato cnunciato left a comment

Choose a reason for hiding this comment

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

Sorry, clicked the Approve option by mistake. (See my previous comment.)

@tmeckel
Copy link
Contributor Author

tmeckel commented Apr 3, 2024

Sorry, clicked the Approve option by mistake. (See my previous comment.)
@cnunciato Too bad 😁

Anyways ... I'll update the docs and remove (translate) or remove the HCL code you mentioned in #4184 (review)

@tmeckel
Copy link
Contributor Author

tmeckel commented Apr 4, 2024

@cnunciato

Before I create a new release of the Fortios provider, can you have a look at the PR in the provider repo pulumiverse/pulumi-fortios#8 if everything has been corrected.

@tmeckel tmeckel requested a review from cnunciato April 4, 2024 13:08
@t0yv0
Copy link
Member

t0yv0 commented Apr 4, 2024

LMK if I'm needed here. Happy to jump on a call.

@cnunciato
Copy link
Contributor

@tmeckel I'm still seeing issues -- left you some comments on the PR. Here's what I see when I render these out:

image

@tmeckel
Copy link
Contributor Author

tmeckel commented Apr 5, 2024

@cnunciato It is no excuse not to work carefully, but the fact that you have to squeeze the documentation into two files with the community providers makes it easy to lose track in huge files.

We already had a discussion about this
#3159 including a PR from my side pulumi/registrygen#19

unfortunately we did not come to a conclusion.

So could you please have another look at the PR in the Fortios provider repo. I really hope that I now fixed all open issues.

@cnunciato
Copy link
Contributor

Thanks for rolling with the revisions, @tmeckel!

@tmeckel
Copy link
Contributor Author

tmeckel commented Apr 5, 2024

@cnunciato created a new PR containing the fixes to the docs
image

@cnunciato cnunciato merged commit 9e55c45 into pulumi:master Apr 5, 2024
3 checks passed
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.

4 participants