-
Notifications
You must be signed in to change notification settings - Fork 117
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
Use pulumi convert to automate conversion of examples #1999
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
* selector: { | ||
* matchLabels: { | ||
* app: "nginx", | ||
* }, | ||
* }, | ||
* serviceName: nginxService.metadata.name, | ||
* replicas: 3, | ||
* serviceName: service.metadata.apply(metadata => metadata?.name), |
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.
Side note: I'm guessing the .metadata.name
property isn't marked as required in the schema, which results in the ugly apply rather than the lifted form.
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 interestingly this is one of the reasons to use pulumi convert over kube2pulumi here - since it allows us to embed resource references and produce more pulumi-esque code.
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.
Loving this change overall! It would also be nice to connect it to the build process, but we could defer that later since the convert function is currently a bit slow.
md := strings.NewReplacer(".yaml", ".md", ".yml", ".md").Replace(base) | ||
|
||
buf := bytes.Buffer{} | ||
_, err = buf.WriteString("{{% examples %}}\n") |
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 think it would be easier to generate the markdown file using a template, but I won't block on this since the automation is already an improvement over the status quo.
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 I thought about that. Definitely worth revisiting this with a template. There are still some rough edges with pulumi convert - as in it tries to install dependencies etc. which may fail. Mostly think of this as a script for now which can be run to update the examples as they are added. This is not even integrated into CI.
contract.IgnoreError(os.RemoveAll(dir)) | ||
}() | ||
|
||
fmt.Fprintf(os.Stderr, "New dir: %q\n", dir) |
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.
Should this be on stderr?
description: Create an Ingress with auto-naming | ||
name: yaml-example | ||
resources: | ||
ingress: |
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 might be helpful to name the resources something different than the type to make the usage clearer.
Related: #2001 |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
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.
Thanks for cleaning this up! This will be very helpful.
targetPort: 9376 | ||
name: yaml-example | ||
runtime: yaml | ||
description: Create a Service with autonaming |
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.
description: Create a Service with autonaming | |
description: Create a Service with auto-naming |
app: nginx | ||
name: yaml-example | ||
runtime: yaml | ||
description: Create a StatefulSet with autonaming |
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.
description: Create a StatefulSet with autonaming | |
description: Create a StatefulSet with auto-naming |
@@ -525,3 +525,32 @@ jobs: | |||
name: lint | |||
if: github.event_name == 'repository_dispatch' || | |||
github.event.pull_request.head.repo.full_name == github.repository | |||
ensure-examples-gen: |
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 don't think we want to add this to the acceptance tests at this time. The convert process is a bit slow, and the examples are currently static. Let's add a Makefile target instead, and then we can update the examples as needed.
Does the PR have any schema changes?Looking good! No breaking changes found. |
/run-acceptance-tests |
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/2657977474 |
Does the PR have any schema changes?Looking good! No breaking changes found. |
2 similar comments
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
We have multiple ways of generating docs, and including docs overlays for the Docker:Image resource was done via this tool. We have since moved to keeping "extra" documentation as part of the provider repository, examples include pulumi/pulumi-databricks#38 and pulumi/pulumi-kubernetes#1999, as well as altering documentation in our terraform provider forks. We have the ability to keep example documentation in the schema for Docker:Image as of pulumi-docker v4, and therefore this functionality is obsolete, and removal is a key step in consolidating how we do supplemental provider documentation.
Proposed changes
This change partially addresses #1981 - i.e. it adds support for yaml but not java which
pulumi convert
doesn't currently support. In the process it automates the conversion of canonical yaml examples to the various languages.Related issues (optional)
#1981