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

Enable Plugin Framework based resources and functions for pulumi-gcp #1075

Merged
merged 16 commits into from
May 30, 2023

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented May 1, 2023

Fixes #1051

Most of the diff in the PR comes from upgrading the bridge to the latest that can be done as a separate PR.

Here is also some context on deploying pulumi/pulumi-terraform-bridge#981 providers with a muxed SDKv2/PF design.

This currently does not have any known blockers and smaller providers are now in prod exercising the necessary code.

Per @iwahbe testing we are unable to test the new datasource due to test account limitations.

@iwahbe iwahbe self-assigned this May 1, 2023
@iwahbe iwahbe marked this pull request as draft May 1, 2023 18:08
@iwahbe iwahbe force-pushed the iwahbe/hybrid-sdk-pf-provider branch from f5936db to d7ce0f8 Compare May 1, 2023 18:12
@iwahbe iwahbe force-pushed the iwahbe/hybrid-sdk-pf-provider branch from 54c43f1 to 86feb0e Compare May 1, 2023 19:10
@github-actions
Copy link

github-actions bot commented May 1, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

@iwahbe
Copy link
Member Author

iwahbe commented May 1, 2023

This change adds a single datasource: getAndroidAppConfig. In an attempt to test the datasource, I tried to deploy an AndroidApp. This is the message I got:

Diagnostics:
  gcp:firebase:AndroidApp (app):
    error: 1 error occurred:
        * Error creating AndroidApp: googleapi: Error 403: Your application has authenticated using end user credentials from the Google Cloud SDK or Google Cloud Shell which are not supported by the firebase.googleapis.com. We recommend configuring the billing/quota_project setting in gcloud or using a service account through the auth/impersonate_service_account setting. For more information about service accounts and how to use them in your application, see https://cloud.google.com/docs/authentication/. If you are getting this error with curl or similar tools, you may need to specify 'X-Goog-User-Project' HTTP header for quota and billing purposes. For more information regarding 'X-Goog-User-Project' header, please check https://cloud.google.com/apis/docs/system-parameters.
    Details:
    [
      {
        "@type": "type.googleapis.com/google.rpc.ErrorInfo",
        "domain": "googleapis.com",
        "metadata": {
          "consumer": "projects/764086051850",
          "service": "firebase.googleapis.com"
        },
        "reason": "SERVICE_DISABLED"
      }
    ]

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

@t0yv0 t0yv0 requested review from a team and kpitzen May 18, 2023 00:38
@t0yv0 t0yv0 marked this pull request as ready for review May 18, 2023 00:40
@t0yv0 t0yv0 changed the title Hybrid sdk pf provider Enable Plugin Framework based resources and functions May 18, 2023
@t0yv0 t0yv0 changed the title Enable Plugin Framework based resources and functions Enable Plugin Framework based resources and functions for pulumi-gcp May 18, 2023
@kpitzen
Copy link
Contributor

kpitzen commented May 18, 2023

Hey @t0yv0 and @iwahbe - thanks for pushing this work forward! Does adding this change the upgrade / release process for the provider re: what's currently in our published playbooks?

@t0yv0
Copy link
Member

t0yv0 commented May 18, 2023

@kpitzen not intentionally. Can you point me to the runbooks I can work with you on that. One obvious change is the new dependency on pulumi-terraform-bridge/pf module, that needs to be kept up to date.

@t0yv0
Copy link
Member

t0yv0 commented May 18, 2023

FYI Ian is on PTO until May 30 but I can answer any questions in the meanwhile.

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

Happy with all the implementation.

Let's split this PR so we've got a simple bridge upgrade PR first so we can see if there's any significant SDK changes.

Looking at we've reverted it looks like only data sources have been implemented in tfpf so far - so we're not seeing any added resources?

Once we're ready to merge, I'd suggest also including into this PR reverting framework revert patch from the fork as part of the upgrade so we don't carry it forward

// Provider returns additional overlaid schema and metadata associated with the gcp package.
func Provider() tfbridge.ProviderInfo {
p := shimv2.NewProvider(google.Provider())
p = pf.MuxShimWithPF(context.Background(), p, google.New(""))
Copy link
Member

Choose a reason for hiding this comment

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

What's the significance of the google.New("") - why the empty string?

Copy link
Member

Choose a reason for hiding this comment

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

It's a version. Perhaps version.Version would be more appropriate here, something we could try.

Looks like the version is used by the pluginFrameworkProvider in particular in responses to GetMetadata function but I doubt we depend on that being accurate.

// New is a helper function to simplify provider server and testing implementation.
func New(version string) provider.ProviderWithMetaSchema {
	return &frameworkProvider{
		version: version,
	}
}

@t0yv0
Copy link
Member

t0yv0 commented May 23, 2023

I'm curious if there's automation to keep the bridge reference up-to-date, it's a chore.

Trying this:

https://github.com/pulumi/pulumi-gcp/actions/runs/5059976598

@t0yv0
Copy link
Member

t0yv0 commented May 23, 2023

Looking at we've reverted it looks like only data sources have been implemented in tfpf so far - so we're not seeing any added resources?

Yes, this is a bit strange; do you know which resources we expect? Is this because some PF resources were poly-filled in a different way? Can you give some pointers?

Once we're ready to merge, I'd suggest also including into this PR reverting framework revert patch from the fork as part of the upgrade so we don't carry it forward

Any pointers?

@t0yv0
Copy link
Member

t0yv0 commented May 23, 2023

#1090 baseline bridge update

@t0yv0
Copy link
Member

t0yv0 commented May 23, 2023

Updated the PR. The diff is much smaller and highlights that's only one method is being added.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

2 similar comments
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

@t0yv0
Copy link
Member

t0yv0 commented May 26, 2023

Per conv with @mikhailshilkov this provider has not been patching PF-based resources. Checks on upstream code. No PF resources:

// Resources defines the resources implemented in the provider.
func (p *frameworkProvider) Resources(_ context.Context) []func() resource.Resource {
	return nil
}

We should be seeing more data sources though:


// DataSources defines the data sources implemented in the provider.
func (p *frameworkProvider) DataSources(_ context.Context) []func() datasource.DataSource {
	return []func() datasource.DataSource{
		NewGoogleClientConfigDataSource,
		NewGoogleClientOpenIDUserinfoDataSource,
		NewGoogleDnsManagedZoneDataSource,
		NewGoogleDnsRecordSetDataSource,
		NewGoogleDnsKeysDataSource,
		NewGoogleFirebaseAndroidAppConfigDataSource,
		NewGoogleFirebaseAppleAppConfigDataSource,
		NewGoogleFirebaseWebAppConfigDataSource,
	}
}

I'l have a closer look as to why we are not seeing those.

@t0yv0
Copy link
Member

t0yv0 commented May 26, 2023

Thanks @danielrbradley for pointing out an upstream commit that patched upstream to serve the missing datasources from SDKv2 versions. I've tried reverting this commit on an upstream branch and building again, hoping this move all the datasources onto PF implementation now.

Revert framework conversions
- Leave the new framework versions in-place.
- Rename the reverted versions to have the filename suffix `_reverted` and the function name suffix `...Sdk`.
- Re-insert into old DataSourcesMap in provider.go

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

@t0yv0
Copy link
Member

t0yv0 commented May 26, 2023

So the 7 functions are not new according to schema-tools check but they're switching over to PF implementations as evidenced in the edit to bridge-metadata.json dispatch mappings.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.

New functions:

  • firebase/getAndroidAppConfig.getAndroidAppConfig

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.

Good to go as far as I am concerned

Copy link
Contributor

@kpitzen kpitzen left a comment

Choose a reason for hiding this comment

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

Apologies for the late reply - agreed that this LGTM

@t0yv0
Copy link
Member

t0yv0 commented May 30, 2023

Couple more quick questions before I merge.

Does the pulumi/terraform-provider-google-beta@e22b067c5b69 change look good?

Should I tag this in the upstream somehow?

@kpitzen
Copy link
Contributor

kpitzen commented May 30, 2023

Couple more quick questions before I merge.

Does the pulumi/terraform-provider-google-beta@e22b067c5b69 change look good?

Should I tag this in the upstream somehow?

This LGTM - we had to do something similar as the upstream TF provider refactored some of the config functionality. Shouldn't be a need to tag upstream.

@t0yv0 t0yv0 merged commit 6e90538 into master May 30, 2023
@t0yv0 t0yv0 deleted the iwahbe/hybrid-sdk-pf-provider branch May 30, 2023 15:42
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.

[TFPF] Upstream provider is now a mixed SDK/TFPF provider
5 participants