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

Include examples with errors #645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Nov 15, 2022

We generate a huge number of examples with the majority of examples correct.

From the perspective of pulumi/pkg/codegen authors, the examples that are not correct can be split into two catagories:

  1. Examples where the PCL is syntactically correct but does not describe a valid program.

  2. Examples where the PCL describes a valid program but the resulting example is not valid.

(2) is a bug in program-gen. I think we all agree that valid PCL should result in a valid program.

(1) is more complicated. Right now, programgen tries to produce an output file that is as close to a valid program as possible. If it believes the output program will not be correct, an error diagnostic explaining why the result will not be valid is emmited with the file.

We need to decide if we want to include examples that are known to be incorrect, but could still be directionally helpful.

We generate a huge number of examples with the majority of examples
correct.

From the perspective of `pulumi/pkg/codegen` authors, the examples that
are not correct can be split into two catagories:

1. Examples where the PCL is syntactically correct but does not describe
a valid program.

2. Examples where the PCL describes a valid program but the resulting
example is not valid.

(2) is a bug in program-gen. I think we all agree that valid PCL should
result in a valid program.

(1) is more complicated. Right now, programgen tries to produce an
output file that is as close to a valid program as possible. If it
believes the output program will not be correct, an error diagnostic
explaining why the result will not be valid is emmited with the file.

We need to decide if we want to include examples that are known to be
incorrect.
@iwahbe iwahbe requested review from t0yv0 and Frassle November 15, 2022 22:21
@iwahbe
Copy link
Member Author

iwahbe commented Nov 15, 2022

I believe that we should include examples with errors. It is the *.GenerateProgram's responsibility to return a non-nil error if the output is unusable. Otherwise we should give a best effort.

@github-actions
Copy link

Diff for pulumi-random with merge commit 1514d13

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 1514d13

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 1514d13

@lblackstone
Copy link
Member

We need to decide if we want to include examples that are known to be incorrect, but could still be directionally helpful.

Some examples that fall into this category include resources that depend on an input that isn't shown in the example. I think that these are still better than no example, so I'd vote to include them if possible.

@t0yv0
Copy link
Member

t0yv0 commented Nov 15, 2022

Can impact assessment be included here? # of examples that this now permits to come through, with type-1 vs type-2? Or we cannot classify type-1/type-2?

@github-actions
Copy link

Diff for pulumi-azure with merge commit 1514d13

@iwahbe
Copy link
Member Author

iwahbe commented Nov 15, 2022

Can impact assessment be included here? # of examples that this now permits to come through, with type-1 vs type-2? Or we cannot classify type-1/type-2?

We can't classify type 1 vs type 2 unfortunately. They both show up as error diagnostics.

@github-actions
Copy link

Diff for pulumi-aws with merge commit 1514d13

@t0yv0
Copy link
Member

t0yv0 commented Nov 15, 2022

Well what are the common classes of errors? Can we recognize them? We can run regex over diagnostics if necessary here. Perhaps this could be recognized:

Some examples that fall into this category include resources that depend on an input that isn't shown in the example. I think that these are still better than no example, so I'd vote to include them if possible.

Could we recognize this case --^

I worry about blindly (without understanding metrics/impact) deciding to publish more known-garbage when we simultaneously aim to raise the bar on documentation quality. Perhaps help me understand what motivates this change or how it's related to the bigger plan of figuring out docs quality?

@thomas11
Copy link
Contributor

There seem to be cases where examples of type (1) are already emitted. pulumi/registry#1686 is a recent example from triage. In that case, the user flagged the example as incorrect.

Maybe we could emit a bit of information so the registry could say something like "incomplete example to illustrate the concept".

@iwahbe
Copy link
Member Author

iwahbe commented Nov 16, 2022

There seem to be cases where examples of type (1) are already emitted. pulumi/registry#1686 is a recent example from triage. In that case, the user flagged the example as incorrect.

Maybe we could emit a bit of information so the registry could say something like "incomplete example to illustrate the concept".

We do emit examples that are type (1), but that is generally itself a type (2) error. In theory, there should always be a type error in the binder when the input program is mistyped.

With regards to pulumi/registry#1686 in particular, pulumi/pulumi#11371 should add the error for that.

I like the idea of the registry containing a "this is a solid example" or "this is a directional example".

@viveklak
Copy link
Contributor

viveklak commented Nov 16, 2022

I like the idea of the registry containing a "this is a solid example" or "this is a directional example".

Would it make sense to record the diags and export them as part of the example? I understand that the diags can sometimes be of dubious quality though. Perhaps a high level heuristic of adding a little caution chevron next to examples with diags is sufficient to suggest the example might need some additional changes...

@t0yv0 t0yv0 requested a review from viveklak November 17, 2022 16:38
@t0yv0
Copy link
Member

t0yv0 commented Nov 17, 2022

On the general question of whether to include the suspect examples or not - I'd trust Vivek's intuition here as he dealt with this some more than I have to assess the tradeoffs. Just added you as a reviewer Vivek!

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.

None yet

5 participants