Use BaseUrl in resources instead of BasePaths in resource templates

[melinath]

Also implemented handling of REP urls with the BaseUrl function.

This should end up choosing/creating the same URLs in all cases, when combined with ReplaceVars location replacement. (The only place that wasn't available was in operation code.)

One side effect of this change is that the BasePaths would end up with different values for REP-enabled products & configs (but I believe that's what we would want anyway, and BasePath usage will be phased out in following PRs in favor of the BaseUrl function.)

cc @SirGitsalot FYI

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

[melinath]

VCR tests failed because the output was too long; excerpt:

Found 4506 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit

That's most of the tests. That shouldn't be the case. I'll need to take a closer look tomorrow.

[melinath]

Ran TestAccApiGatewayApiConfig_apigatewayApiConfigFullExample VCR replaying locally and got the error:

Error: Error creating Api: Post "https://apigateway.googleapis.com/v1beta/projects/%7B%7Bproject%7D%7D/locations/global/apis%3FapiId=%7B%7Bapi_id%7D%7D?alt=json": Requested interaction not found

So for some reason, the ReplaceVars isn't working as expected.

[melinath]

So, I had used url.JoinPath based on a conversation with Riley; since these are URLs, it would be more "proper" / correct to join them with the appropriate function. Unfortunately, it turns out that url.JoinPath automatically encodes the result, which means that the {} characters were converted to %7B%7D, which meant that ReplaceVars would then fail to replace anything.

We could do replacement on the two URL sections separately, then join them, or we could join them, unescape them, and then do the replacement - but for now, I think it's simpler to just continue the practice of joining them as strings.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1416 files changed, 5076 insertions(+), 5006 deletions(-))
google-beta provider: Diff ( 1568 files changed, 5567 insertions(+), 5474 deletions(-))
terraform-google-conversion: Diff ( 5 files changed, 18 insertions(+), 217 deletions(-))

[melinath]

TGC integration failure looks unrelated.

[modular-magician]

Tests analytics

Total tests: 6364
Passed tests: 5688
Skipped tests: 652
Affected tests: 24

Click here to see the affected service packages

All service packages are affected

Action taken

Found 24 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccApigeeOrganization_apigeeOrganizationCloudBasicDisableVpcPeeringTestExample
• TestAccApigeeOrganization_apigeeOrganizationCloudFullDisableVpcPeeringTestExample
• TestAccBeyondcorpAppConnection_beyondcorpAppConnectionBasicExample
• TestAccCloudRunService_cloudRunServiceGpuExample
• TestAccContainerCluster_updateVersion
• TestAccContainerCluster_withAutopilotResourceManagerTags
• TestAccContainerNodePool_withHostMaintenancePolicy
• TestAccDataSourceGoogleCloudRunService_basic
• TestAccDataSourceGoogleCloudRunService_optionalProject
• TestAccDataSourceGoogleDiscoveryEngineDataStore_basic
• TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_basicWithParameterName
• TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_basicWithResourceReference
• TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_withJsonData
• TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_withKmsKey
• TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_withYamlData
• TestAccDataSourceParameterManagerRegionalRegionalParameter_basic
• TestAccDataSourceSecretManagerRegionalRegionalSecret_basic
• TestAccDataformConfig_update
• TestAccDataformRepository_dataformRepositoryWithCloudsourceRepoAndSshExample
• TestAccDataprocMetastoreService_dataprocMetastoreServicePrivateServiceConnectExample
• TestAccDiscoveryEngineDataStore_discoveryengineDatastoreKmsKeyNameExample
• TestAccManagedKafkaConnector_managedkafkaConnectorBasicExample
• TestAccProjectIamMemberRemove_memberInMultipleBindings
• TestAccPubsubSubscription_pubsubSubscriptionTagsExample

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccDataSourceGoogleDiscoveryEngineDataStore_basic [Debug log]
TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_basicWithParameterName [Debug log]
TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_basicWithResourceReference [Debug log]
TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_withJsonData [Debug log]
TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_withKmsKey [Debug log]
TestAccDataSourceParameterManagerRegionalRegionalParameterVersion_withYamlData [Debug log]
TestAccDataSourceParameterManagerRegionalRegionalParameter_basic [Debug log]
TestAccDataSourceSecretManagerRegionalRegionalSecret_basic [Debug log]
TestAccDataformConfig_update [Debug log]
TestAccProjectIamMemberRemove_memberInMultipleBindings [Debug log]

🔴 Tests failed when rerunning REPLAYING mode:
TestAccDataformConfig_update [Error message] [Debug log]
TestAccProjectIamMemberRemove_memberInMultipleBindings [Error message] [Debug log]

Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made.

Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.

---

🔴 Tests failed during RECORDING mode:
TestAccApigeeOrganization_apigeeOrganizationCloudBasicDisableVpcPeeringTestExample [Error message] [Debug log]
TestAccApigeeOrganization_apigeeOrganizationCloudFullDisableVpcPeeringTestExample [Error message] [Debug log]
TestAccBeyondcorpAppConnection_beyondcorpAppConnectionBasicExample [Error message] [Debug log]
TestAccCloudRunService_cloudRunServiceGpuExample [Error message] [Debug log]
TestAccContainerCluster_updateVersion [Error message] [Debug log]
TestAccContainerCluster_withAutopilotResourceManagerTags [Error message] [Debug log]
TestAccContainerNodePool_withHostMaintenancePolicy [Error message] [Debug log]
TestAccDataSourceGoogleCloudRunService_basic [Error message] [Debug log]
TestAccDataSourceGoogleCloudRunService_optionalProject [Error message] [Debug log]
TestAccDataformRepository_dataformRepositoryWithCloudsourceRepoAndSshExample [Error message] [Debug log]
TestAccDataprocMetastoreService_dataprocMetastoreServicePrivateServiceConnectExample [Error message] [Debug log]
TestAccDiscoveryEngineDataStore_discoveryengineDatastoreKmsKeyNameExample [Error message] [Debug log]
TestAccManagedKafkaConnector_managedkafkaConnectorBasicExample [Error message] [Debug log]
TestAccPubsubSubscription_pubsubSubscriptionTagsExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[melinath]

The VCR test failures look unrelated to me; problems with this code change would surface as 404s (due to the base URL being empty or incorrect).

Most of the failures are failures on all PRs (that impact the service) - compare for example #17325.

The new failures here are TestAccDataSourceGoogleCloudRunService_basic and TestAccDataSourceGoogleCloudRunService_optionalProject. The URLs look correct, and I can get them to pass locally with this PR (though they're a bit flakey).

So that's all to say: @slevenick this is ready for review!

[github-actions]

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[slevenick]

It could make sense to unguard this as having {{location}} in any url is probably a failure mode

[melinath]

TBH we might want to flag it as a failure mode if any {} characters are present in a URL... or maybe that should be an error returned by ReplaceVars if there's any replacement that didn't happen?

In either case - I'd be happy to make that change, but it'll probably be as a separate PR for clarity / to avoid blocking this one.

[slevenick]

Sounds good to me. Putting it in ReplaceVars would make sense, but I have a suspicion it would trigger errors on places we've handled replacement afterwards.

[melinath]

PR looking into this: #17419