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

Remove the need to specify api/ files, pull directly from the DCL #5197

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

rileykarson
Copy link
Member

@rileykarson rileykarson commented Sep 14, 2021

Fixes hashicorp/terraform-provider-google#8201

This PR removes the need to specify api/ files for resources (although not samples quite yet) and pulls them directly off of the DCL's exported YAMLs. This means we'll stay up-to-date for better or for worse, although generally for the better. There's some pain in the upgrade- our YAMLs had drifted, and there were some dangerous-to-apply updates last I checked.

Changes are contained to main.go, and all other changes in the PR are non-functional (removing api files, removing old overrides)

Marked as WIP because of the drift- we need to resolve those to compatible before we can merge this. Outstanding issues:

  • CloudBuild WorkerPool bad name description: b/200841783
  • CloudBuild WorkerPool update: b/199915504
  • CloudBuild WorkerPool WorkerConfig O+C: cl/399473374
  • Dataproc WorkflowTemplate: b/199937950
  • Dataproc WorkflowTemplate version: Update Dataproc WorkflowTemplate, rename REQUIRED_OVERRIDE #5249
  • EventArc Trigger descriptions: cl/396655269
  • GKEHub Feature config_membership going required: Not believed to be a problem, as the field is effectively required already
  • GKEHub FeatureMembership typo: Fixed upstream, will be picked up after merging with Add make command to upgrade DCL, use it to see if it works. #5201
  • GKEHub FeatureMembership configmanagement/location/feature/membership going required: Not believed to be a problem, same as for Feature
  • PrivateCA CertificateTemplate losing state hint: Spoke with Nat, not believed to be a problem

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

gke_hub: made the `config_membership` field in `google_gke_hub_feature` required, disallowing invalid configurations
gke_hub: made the `configmanagement`, `feature`, `location`, `membership` fields in `google_gke_hub_feature_membership` required, disallowing invalid configurations
eventarc: added support for `uid` output field, `cloud_function` destination to `google_eventarc_trigger`
gke_hub: added support for `resource_state`, `state` outputs to `google_gke_hub_feature`
gke_hub: added support for `gcp_service_account_email` when configuring Git sync in `google_gke_hub_feature_membership`

@google-cla google-cla bot added the cla: yes label Sep 14, 2021
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 163 insertions(+), 226 deletions(-))
Terraform Beta: Diff ( 10 files changed, 327 insertions(+), 309 deletions(-))

@rileykarson rileykarson requested review from slevenick and removed request for slevenick September 14, 2021 18:42
@rileykarson
Copy link
Member Author

Actually, wait, found a couple more cleanups I wanted to make.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 163 insertions(+), 226 deletions(-))
Terraform Beta: Diff ( 9 files changed, 326 insertions(+), 308 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCloudbuildWorkerPool_basic|TestAccComputeForwardingRule_update|TestAccDataprocWorkflowTemplate_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=205770

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode: TestAccCloudbuildWorkerPool_basic Please fix these to complete your PR

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 170 insertions(+), 233 deletions(-))
Terraform Beta: Diff ( 11 files changed, 334 insertions(+), 275 deletions(-))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 240 insertions(+), 227 deletions(-))
Terraform Beta: Diff ( 11 files changed, 404 insertions(+), 269 deletions(-))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 166 insertions(+), 235 deletions(-))
Terraform Beta: Diff ( 11 files changed, 331 insertions(+), 277 deletions(-))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 171 insertions(+), 87 deletions(-))
Terraform Beta: Diff ( 11 files changed, 334 insertions(+), 121 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCloudbuildWorkerPool_basic|TestAccComputeServiceAttachment_serviceAttachmentBasicExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=207256

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode: TestAccCloudbuildWorkerPool_basic Please fix these to complete your PR

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Looks fine to me once all the DCL resources are fixed!

tpgtools/main.go Outdated Show resolved Hide resolved
tpgtools/serialization.go Show resolved Hide resolved
@rileykarson
Copy link
Member Author

@slevenick any concern if I squash down to one commit when I resolve those merge conflicts? Tracking go.mod changes across a few different commits is annoying to rebase.

@slevenick
Copy link
Contributor

@slevenick any concern if I squash down to one commit when I resolve those merge conflicts? Tracking go.mod changes across a few different commits is annoying to rebase.

No concerns! It happens on merge anyways so 🤷

@rileykarson rileykarson changed the title [WIP] Remove the need to specify api/ files, pull directly from the DCL Remove the need to specify api/ files, pull directly from the DCL Sep 27, 2021
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 126 insertions(+), 84 deletions(-))
Terraform Beta: Diff ( 11 files changed, 288 insertions(+), 118 deletions(-))

@rileykarson
Copy link
Member Author

VCR failed to a TC response and killed the other steps >:(

/gcbrun

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 126 insertions(+), 84 deletions(-))
Terraform Beta: Diff ( 11 files changed, 288 insertions(+), 118 deletions(-))

@rileykarson
Copy link
Member Author

Third try's the charm! /gcbrun

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 126 insertions(+), 84 deletions(-))
Terraform Beta: Diff ( 10 files changed, 287 insertions(+), 117 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCloudbuildWorkerPool_basic|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=207803

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode: TestAccCloudbuildWorkerPool_basic|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample Please fix these to complete your PR

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 18 insertions(+), 19 deletions(-))
Terraform Beta: Diff ( 10 files changed, 190 insertions(+), 50 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCloudbuildWorkerPool_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=208023

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 20 insertions(+), 19 deletions(-))
Terraform Beta: Diff ( 9 files changed, 191 insertions(+), 49 deletions(-))

@slevenick slevenick self-requested a review September 29, 2021 21:41
Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

LGTM assuming tests pass!

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 22 insertions(+), 19 deletions(-))
Terraform Beta: Diff ( 10 files changed, 192 insertions(+), 47 deletions(-))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 22 insertions(+), 19 deletions(-))
Terraform Beta: Diff ( 10 files changed, 192 insertions(+), 47 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerCluster_withAuthenticatorGroupsConfig You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=208415

@rileykarson
Copy link
Member Author

Weird- VCR ran, the Magician didn't post a message on the initial run, and it did on the recording. I confirmed the build #s corresponded to the right TPGB commit, too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opt-in resources to tpgtools generation using override files
3 participants