-
Notifications
You must be signed in to change notification settings - Fork 43
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
Download upstream docs for dynamically bridged provider #2664
Conversation
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 left a couple of nits, but I have 1 more major piece of design feedback:
Instead of introducing XLoadUpstreamRepoForDocs
and then threading the logic into the docs generator, could we download the docs during the GetSchema call directly in dynamic/main.go
and then just set UpstreamRepoPath
? That should accomplish the same effect without adding more logic to the docs generator.
4e3bd12
to
27033ba
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2664 +/- ##
=======================================
Coverage 69.53% 69.53%
=======================================
Files 301 301
Lines 38646 38655 +9
=======================================
+ Hits 26871 26878 +7
- Misses 10254 10257 +3
+ Partials 1521 1520 -1 ☔ View full report in Codecov by Sentry. |
064910c
to
6bea03d
Compare
b194a78
to
5cbcf28
Compare
dynamic/testdata/TestSchemaGenerationFullDocs/hashicorp/random-3.6.3.golden
Outdated
Show resolved
Hide resolved
292ae9d
to
f80717e
Compare
This may not be the way we want to do this, but I changed the function signature of Alternatives:
Unfortunately, adding extra options to the GeneratorOpts only gets us halfway - we must call Generate() or its parts somewhere in I've gone with changing the function signature to avoid writing yet another generator function with a narrowly scoped purpose, but can definitely be convinced otherwise. |
332f450
to
da509c8
Compare
To be good citizens of the registry, we should set the `Repository` field. Related to #2664 (comment).
fa56e6a
to
97a7b57
Compare
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.
There are currently two argument styles added in this PR:
pulumi package get-schema terraform-provider ./terraform-provider-local upstreamRepoPath=./local
pulumi package get-schema terraform-provider my/provider fullDocs
One is positional, but the other is flag based. Can we standardize? Personally, I'd prefer flags. That would make it:
pulumi package get-schema terraform-provider ./terraform-provider-local upstreamRepoPath=./local
pulumi package get-schema terraform-provider my/provider docs=full
@@ -6,7 +6,7 @@ export PULUMI_DISABLE_AUTOMATIC_PLUGIN_ACQUISITION := true | |||
PROJECT_DIR := $(patsubst %/,%,$(dir $(abspath $(lastword $(MAKEFILE_LIST))))) | |||
|
|||
install_plugins:: | |||
pulumi plugin install converter terraform 1.0.19 | |||
pulumi plugin install converter terraform 1.0.20 |
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.
Do we need this bump, or is it incidental?
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.
We need this bump, otherwise the test won't pass.
dynamic/provider_test.go
Outdated
type testCase struct { | ||
name string | ||
version string | ||
fullDocs bool | ||
} | ||
|
||
tc := testCase{ | ||
name: "hashicorp/random", | ||
version: "3.6.3", | ||
fullDocs: true, | ||
} |
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.
Why are we defining a local struct only to instantiate one copy? Same with the subtest.
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.
Why are we defining a local struct only to instantiate one copy?
- Test information at a glance
- More easily add subtests in the future if needed
- Allows us to run individual tests by name when using an integrated IDE test framework
- Makes the name of subtest far more obvious for running it in the terminal
Is there a downside 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.
Verbosity and the chance of rot. It's an abstraction we aren't using.
df6f314
to
1bdcf20
Compare
dynamic/parameterize/args.go
Outdated
@@ -61,6 +102,6 @@ func ParseArgs(args []string) (Args, error) { | |||
remote.Name = args[0] | |||
return Args{Remote: &remote}, nil | |||
default: | |||
return Args{}, fmt.Errorf("expected to be parameterized by 1-2 arguments: <name> [version]") | |||
return Args{}, fmt.Errorf("expected to be parameterized by 1-3 arguments: <name> [version] [fullDocs]") |
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.
Let's be consistent with upstreamRepoPath
for local providers.
return Args{}, fmt.Errorf("expected to be parameterized by 1-3 arguments: <name> [version] [fullDocs]") | |
return Args{}, fmt.Errorf("expected to be parameterized by 1-3 arguments: <name> [version] [fullDocs=<true|false>]") |
…nput parameters. Needs refinement.
…dd test for full docsgen
…tion. Skip example conversion when fullDocs is not set.
…Local and Remote arguments.
50ea527
to
cbbd648
Compare
@@ -1,8 +1,8 @@ | |||
{ | |||
"name": "b2", | |||
"version": "0.8.9", |
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 is a regression.
This PR has been shipped in release v3.98.0. |
This pull request adds logic that enables us to generate resource docs for a dynamically bridged provider using the upstream dependency.
Usage for remote providers:
pulumi package get-schema terraform-provider <registry address> <version> fullDocs=<true|false>
.Adds a parameterized arg field to
pulumi package get-schema terraform-provider <registry address> <version>
calledfullDocs
, which when set totrue
will instruct the bridge togit clone --depth 1 -b <version> <terraform provider github repo> <local dir for dynamic docs>
. This allows us to keep the exact same docs logic we have established. The shallow clone targets the exact doc version we need. (*)For this, we infer the github repo from the OpenTofu org name. It is based on the assumption that
registry.opentofu.org/org/foo
is based on a provider that lives atgithub.com/org/terraform-provider-foo
. OpenTofu says their protocol follows that of the HashiCorp Terraform Registry which requires an org/user and a provider name of the formatterraform-provider-foo
and we have historical evidence that GitHub is the most commonly used source host for terraform providers. See also opentofu/registry#1337.Usage for local providers:
pulumi package get-schema terraform-provider <local-path> upstreamRepoPath=<localPath>
. Here, the local docs path gets read directly intoupstreamRepoPath
.The one thing that is perhaps missing here is to expand the remote args to allow to take a source repo as an additional argument. However, our internal use case is primarily one of automation, so this is not an immediate need.
(*)Cloning docs for the AWS terraform provider at v5.70.0 has the following time performance:
Fixes #2607