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

Ensure validate called during tfgen #1854

Merged
merged 14 commits into from
Apr 15, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Apr 11, 2024

fixes #1852

#1669 removed the runtime calls to InternalValidate to speed up provider startup. This assumed it was called during tfgen but it turns out it was not.

This introduces an explicit call to InternalValidate during tfgen. Thanks to @t0yv0 for prompting the investigation and @iwahbe for helping with the muxer.

To facilitate that, I've added InternalValidate to the shim Provider interface. Technically breaking but seems unlikely to be depended on externally.

Calling Validate does not do the job as that requires passing in a valid config which is not available during tfgen.

I've added an e2e test for an sdk provider, need to add one for a pf provider.

@VenelinMartinov VenelinMartinov marked this pull request as draft April 11, 2024 11:07
Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 79 lines in your changes are missing coverage. Please review.

Project coverage is 60.05%. Comparing base (a08acef) to head (307407c).
Report is 8 commits behind head on master.

Files Patch % Lines
internal/testprovider_invalid_schema/provider.go 0.00% 35 Missing ⚠️
internal/testprovider_invalid_schema/resources.go 0.00% 9 Missing ⚠️
pf/tfgen/main.go 0.00% 9 Missing ⚠️
pkg/tfgen/main.go 0.00% 6 Missing ⚠️
pkg/tfgen/generate.go 0.00% 3 Missing ⚠️
...lid_schema/cmd/pulumi-resource-tpinvschema/main.go 0.00% 2 Missing ⚠️
...nvalid_schema/cmd/pulumi-tfgen-tpinvschema/main.go 0.00% 2 Missing ⚠️
pf/internal/schemashim/provider.go 0.00% 2 Missing ⚠️
pkg/tfshim/schema/provider.go 0.00% 2 Missing ⚠️
pkg/tfshim/sdk-v1/provider.go 0.00% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
- Coverage   60.48%   60.05%   -0.43%     
==========================================
  Files         310      323      +13     
  Lines       42835    43894    +1059     
==========================================
+ Hits        25908    26360     +452     
- Misses      15454    16042     +588     
- Partials     1473     1492      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VenelinMartinov VenelinMartinov marked this pull request as ready for review April 11, 2024 13:02
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

This looks good. I think we will need to test on downstream providers before we can merge

pkg/tests/main_test.go Outdated Show resolved Hide resolved
pkg/tfgen/generate.go Outdated Show resolved Hide resolved
VenelinMartinov and others added 3 commits April 11, 2024 15:06
Co-authored-by: Ian Wahbe <ian@wahbe.com>
Co-authored-by: Ian Wahbe <ian@wahbe.com>
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Apr 11, 2024

Downstream failures:

auth0: https://github.com/pulumi/pulumi-auth0/actions/runs/8648328389/job/23711873275?pr=493 - this looks related.

Hitting #1816 - these changes cause failures in tfgen which is not picked up by the downstream check script. Will fix that before continuing here.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Apr 12, 2024

After fixing #1816 it turns out this caused quite a few downsteam failures:

FAILURE https://github.com/pulumi/pulumi-auth0/pull/493
FAILURE https://github.com/pulumi/pulumi-oci/pull/431
MISSING https://github.com/pulumi/pulumi-opsgenie/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-splunk/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-libvirt/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-minio/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-kafka/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-aws/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-onelogin/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-cloudamqp/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-rabbitmq/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-newrelic/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-dnsimple/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-nomad/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-mailgun/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-mysql/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-sumologic/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-vsphere/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-keycloak/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-slack/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-vault/actions/workflows/upgrade-bridge.yml

Looking into it, the ones I checked don't look like actual regressions in the schema but rather an issue with the validation logic.

EDIT: The issue was that Validate actually requires passing in a valid config and for these providers the empty config is invalid. I've changed tfgen to directly call InternalValidate instead.

VenelinMartinov added a commit that referenced this pull request Apr 12, 2024
fixes #1816

Hit it during
#1854

It now prints links for the repos which don't have PRs for the commit at
all:


```
FAILURE pulumi/pulumi-auth0#493
FAILURE pulumi/pulumi-oci#431
MISSING https://github.com/pulumi/pulumi-opsgenie/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-splunk/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-libvirt/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-minio/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-kafka/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-aws/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-onelogin/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-cloudamqp/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-rabbitmq/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-newrelic/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-dnsimple/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-nomad/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-mailgun/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-mysql/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-sumologic/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-vsphere/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-keycloak/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-slack/actions/workflows/upgrade-bridge.yml
MISSING https://github.com/pulumi/pulumi-vault/actions/workflows/upgrade-bridge.yml
```
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Apr 12, 2024

I've kicked off another round of downstream tests from 7d378f7, will report back here once they finish.

Everything except auth0, gitlab and aws passed and the first two failed due to master being broken.

@t0yv0
Copy link
Member

t0yv0 commented Apr 12, 2024

pf may use different mechanics for validations so not as relevant in this PR.

pkg/tests/main_test.go Outdated Show resolved Hide resolved
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

Great!

@VenelinMartinov VenelinMartinov enabled auto-merge (squash) April 15, 2024 08:26
@VenelinMartinov VenelinMartinov merged commit 45d8f28 into master Apr 15, 2024
11 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/ensure_validate_called_during_tfgen2 branch April 15, 2024 08:45
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.

Ensure InternalValidate is called at build time
3 participants