-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add API hub product, with resource ApiHubInstance #12499
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
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @roaks3, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 1 week. Please take a look! Use the label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but will need some more tests
vars: | ||
instance_id: test-instance | ||
autogen_async: true | ||
exclude_delete: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly surprising, can the instance not be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, currently we do not support delete for this resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I'm understanding, only one of these resources can be created per project, it is immutable, and it cannot be deleted, meaning that once an instance is created for a particular project, you can never change it, and you can't touch the Terraform config unless project
is changed (because TF will attempt a re-creation, which won't work).
Is this the intended behavior? It seems a little confusing to a user to not be able to adjust their config, so perhaps we want to mention that a new project must be used somewhere in the documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the description of the resource, mentioning that the resource cannot be deleted/updated as of now.
@@ -0,0 +1,8 @@ | |||
resource "google_apihub_api_hub_instance" "{{$.PrimaryResourceId}}"{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need some more tests to check the range of config that can be provided (see https://googlecloudplatform.github.io/magic-modules/test/test/):
- Since this test should be minimal, is
config {}
valid? - You'll need an "update" test to confirm fields that are mutable can be updated in-place, like description and config
- You'll need to use each field in at least one test (you can do this with the update test, or add an explicit
*_full.tf.tmpl
example with every possible field specified)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- config{} wont be valid. One of disable_search or vertex_location would need to be provided. Default value of disable_search would be false. But in that scenario, vertex_location would need to be provided in the config.
- Currently update is also not supported for this resource
- Currently only one resource/project can be provisioned. And since delete and update is not supported for this resource, was not able to add further test in the same flow. Do you have any suggestions for this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a test with all possible fields, which will be provisioned in a separate project. Hope that is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both tests, could you create a separate project for the instance? Otherwise our test environments will become polluted in a way we seemingly cannot clean up. Ie.:
resource "google_project" "project" {
name = "{{index $.Vars "project_id"}}"
project_id = "{{index $.Vars "project_id"}}"
org_id = "{{index $.TestEnvVars "org_id"}}"
deletion_policy = "DELETE"
}
And in the examples blocks:
vars:
project_id: 'apihub-proj'
test_env_vars:
org_id: 'ORG_ID'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, currently using exclude_test. Have created a separate bug and once one of the pre-requisite bug is competed, will remove this flag.
cb48e98
to
fbca264
Compare
@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
@edwinfranciss, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, and I'd be curious if you have any more detail regarding the PreConfig option, but otherwise LGTM
@@ -0,0 +1,15 @@ | |||
resource "google_apihub_api_hub_instance" "{{$.PrimaryResourceId}}"{ | |||
project = "ah-terraform-provider-1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You are excluding this test, but it will still show up as an example in the documentation. We may want to tweak this just for clarity to something like my-project
here and in cmek_key_name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated the project used in the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, there were issues when calling the 1P API using the PreConfig option. The tests will be updated once we have the delete functionality for the resource as well.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_apihub_api_hub_instance" "primary" {
api_hub_instance_id = # value needed
config {
cmek_key_name = # value needed
disable_search = # value needed
encryption_type = # value needed
vertex_location = # value needed
}
description = # value needed
labels = # value needed
location = # value needed
}
|
Tests analyticsTotal tests: 4447 Click here to see the affected service packages
Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: 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: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
This PR creates support for new GCP product API hub - https://cloud.google.com/apigee/docs/apihub/what-is-api-hub
As part of the product, added support for the resource ApiHubInstance (google_apihub_api_hub_instance)
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.