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

Compute schema-minimal.json #4587

Merged
merged 18 commits into from
Oct 7, 2024
Merged

Compute schema-minimal.json #4587

merged 18 commits into from
Oct 7, 2024

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Oct 1, 2024

Perform automatic rewrites over the schema to remove dangling type references and produce a minimal schema for tool consumption that is distinct from the real schema fed into SDK generation. The "minimal" schema has these properties:

  • it has no dangling type references
  • it accurately describes the provider's SDKs for Python, Go, Java, C# and YAML
  • it approximately describes the provider's Node SDK behavior

Specifically for Node, the minimal schema may say that a string is accepted where string + concrete type union can actually be used in Node.

By default the minimal schema is not load bearing. It is possible to activate it by setting an environment variable:

PULUMI_AWS_MINIMAL_SCHEMA=true

With this set, pulumi-resource-aws will be serving the minimal schema from the GetSchema() gRPC method.

Also it is possible to generate SDKs based off the minimal schema. Currently this generates exact same SDKs as the normal process for Python, Go, .NET and Java, for example:

PULUMI_AWS_MINIMAL_SCHEMA=true make tfgen_build_only build_python

Generating Node SDK off the minimal schema produces slight changes. These may break user programs and we do not want to ship this as the main schema just yet. We intend to reconcile them and remove the distinct minimal schema at the next major version of the provider.

Re: #2565

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 1, 2024

Iterating to make tests happy.

Copy link

github-actions bot commented Oct 1, 2024

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 1, 2024

Might need #4589 first to get schema in-line with the expected one.

Copy link
Member

@mikhailshilkov mikhailshilkov left a comment

Choose a reason for hiding this comment

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

This makes sense to me. A few questions:

  1. It looks like it's not specific to AWS - should we find a shared library to put the rewrite function?
  2. What happens if we generate non-node SDKs with the light schema? Will there be any changes? I would hope for a yes. If so, should we actually generate those SDKs from this schema?
  3. What are the downsides (if any) of servicing this light schema from provider's gRPC?

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 3, 2024

  1. The patterns are pretty AWS-specific, do you have more providers with the same problem? I can poke to see if it's generalization-worthy, otherwise would not bother. There is a pulumi schema check command that could be enhanced to lint dangling references, or do pulumi schema check --fix perhaps if we want to take this on.

  2. That's a good reminder, I wanted to try this out but lost the reminder note, indeed I'll try and see, the expectation is that all the other SDKs behave just the same. Regarding actually switching the generators to use that permanently, do you see benefits outweighing change friction? It makes schema-light more load bearing which may helps us find further issues; but that is also the downside of schema-light becoming load-bearing. Is there at all a possibility to iterate with the customer a bit first to see if schema-light solves their problem and is therefore worth supporting? Then possibly rely on it more. Thanks.

  3. That's a great question! I pondered this and I hesitate because I do not know enough on what is consuming that. (e..g does registry?). To make changes there safely I'm missing a place where we can document what various consumers of GetSchema expect.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 3, 2024

At a very high-level Mikhail I'm thinking of schema-light as a semi-temporary workaround until we can take the breaks in V7 major, so my default assumption is to invest minimally here to meet the requirements and not over-do it. Let's talk if we envision something more permanent here.

Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

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

Looks like a great temp workaround with enough guard rails that we're not gonna regress here. Would be great to see if we can build the SDKs with that

"github.com/stretchr/testify/require"
)

func TestNoDanglingReferencesInLightSchema(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Good way to ensure we're not regressing!

provider/schema_light.go Outdated Show resolved Hide resolved
@mikhailshilkov
Copy link
Member

Agree with all your responses, thank you. One problem is that Cosmic expects to consume this schema via a CLI command, so they will not see the light schema. I'm not sure they are open to changing their tooling to retrieve our schema from our GitHub. But we can ask.

I think it's a good suggestion to let them try the result before shipping it. Is this PR ready for me to point them to?

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 3, 2024

That is interesting new bit of information, thank you! In that case I can embed the light schema in the provider and make it available through GetSchema() but perhaps under environment variable flag for starters. We can promote this to the default behavior once it matures and we decide it's safe and beneficial.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 3, 2024

This PR has schema-light.json but I need to add the ability to serve it off GetSchema(). Working on it presently.

@t0yv0 t0yv0 force-pushed the t0yv0/fix-dangling-refs-2 branch from 72f93d5 to bb90adc Compare October 3, 2024 20:37
@t0yv0
Copy link
Member Author

t0yv0 commented Oct 3, 2024

Re-generating SDKs other than Node is coming back clean. That's good. Added support for versioning and embedding the light schema in the provider, and dispatching to it through a flag. I might need to fight the tests and CI some more, but PTAL again on the approach.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 4, 2024

Once I get tests to pass I will re-brand this as "minimal". Thanks.

t0yv0 added a commit to pulumi/ci-mgmt that referenced this pull request Oct 4, 2024
The standard workflows are great, but sometimes build-time complexity is a necessary evil for temporary or experimental
features. One concrete use case is simplifying computing the minimal schema in AWS:

     pulumi/pulumi-aws#4587

This small change permits providers to define *.mk files to add some provider-specific extensions to Make.
github-merge-queue bot pushed a commit to pulumi/ci-mgmt that referenced this pull request Oct 4, 2024
The standard workflows are great, but sometimes build-time complexity is
a necessary evil for temporary or experimental features. One concrete
use case is simplifying computing the minimal schema in AWS:

     pulumi/pulumi-aws#4587

This small change permits providers to define *.mk files to add some
provider-specific extensions to Make.
@t0yv0 t0yv0 changed the title Compute schema-light.json Compute schema-minimal.json Oct 4, 2024
@t0yv0 t0yv0 marked this pull request as ready for review October 5, 2024 00:10
- name: Install Go
uses: actions/setup-go@v5
with:
go-version: 1.22.x
Copy link
Contributor

Choose a reason for hiding this comment

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

The provider is using 1.23 now

Check that minimal schema is up-to-date.

on:
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also run that as part of the release process? That would guard against releasing an inconsistent schema (e.g. when mismerges happen)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I will follow up. I cannot test this workflow before it merges, I'll need some more PRs to get this right.

@t0yv0 t0yv0 enabled auto-merge (squash) October 7, 2024 17:00
@t0yv0 t0yv0 merged commit 39c3e7d into master Oct 7, 2024
30 checks passed
@t0yv0 t0yv0 deleted the t0yv0/fix-dangling-refs-2 branch October 7, 2024 18:59
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v6.56.0.

t0yv0 added a commit that referenced this pull request Nov 26, 2024
Keeping a copy of minimal schema (introduced in
#4587) is proving to conflict
with Pulumi tooling such as `upgrade-provider` and is not strictly
necessary. With this change the minimal schema itself as well as CI
checks to make sure it is up to date are removed. Instead it will be
computed on-the-fly by the release job as before:
 
- GitHub actions invoke this:
https://github.com/pulumi/pulumi-aws/blob/916e6f28039407d7dcb1a295c6f9e43613dd6f87/.github/workflows/build_provider.yml#L60

- Make extension ensures that this runs go generate prior to the actual
build so that the minimal schema is re-computed:
https://github.com/pulumi/pulumi-aws/blob/916e6f28039407d7dcb1a295c6f9e43613dd6f87/.mk/minimal_schema.mk#L9

Fixes:

- #4811
- #4702
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