-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Adds UnitKind provider for SaasRuntime #14910
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
Adds UnitKind provider for SaasRuntime #14910
Conversation
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @hao-nan-li, 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. |
|
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_saas_runtime_unit_kind" "primary" {
annotations = # value needed
default_release = # value needed
input_variable_mappings {
to {
ignore_for_lookup = # value needed
}
}
labels = # value needed
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
|
Tests analyticsTotal tests: 4 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Regarding the Missing test report: Default release is a field that refers to a resource that we haven't added a provider for, and I'm following the guideline to add one resource at a time. I can add both resources in this PR if preferred so I can have full test coverage. Both resources depend on each other but the expected flow is 1) Create UnitKind, 2) Create Release with required UnitKind association, 3) Optionally set the default release on UnitKind to the release. For OutputVariableMappings, although both input and output variable mappings have the same type, in practice output only uses "from" mapping as I've added in the test. The "to" mapping in output variables is not used. |
|
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_saas_runtime_unit_kind" "primary" {
default_release = # value needed
input_variable_mappings {
to {
ignore_for_lookup = # value needed
}
}
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
|
Tests analyticsTotal tests: 4 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Thanks for the explanation, if the |
The issue I see is that the UnitKind provider without the "default release" field is much less useful. If it's critical to test this field, and if it's ok with you, I would prefer to add both providers in this PR than remove a critical field from this resource and add it later. To explain why: Releases are typically created as part of a CI/CD pipeline outside of Terraform. Then you might update the UnitKind terraform configuration to point to a particular release ID, without having the Release itself managed through Terraform. So UnitKind without default release will not satisfy customer needs. |
|
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_saas_runtime_unit_kind" "primary" {
default_release = # value needed
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
|
Tests analyticsTotal tests: 4 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
I'm not sure I can fully understand what you said, but go ahead if you believe adding both providers is beneficial. |
|
While adding the Release provider, I ran into an issue because it requires an Artifact Registry Image that is built in a very specific way. I found https://yaqs.corp.google.com/eng/q/6819086453071413248 which mentions that you can assist with this. I tried creating the resource in our own testing project and granting the Terraform test project service agent the necessary permissions, but I ran into the issue documented in go/terraform-team-testing#other-dependencies:
It's also not possible to create this resource through Terraform, so I can't have it set up as part of the test. If I'm understanding correctly, there are some shared testing projects (like tf-static-<>) that are used to host static resources. Would it be possible for me to get access to one of those to create the images myself? Or if you're able to do it, can you create the images directly in the testing projects? The instructions are here:go/ez-experience:gcloud-cli#create-blueprints. |
Do you mind setting up a quick 15 minutes with me? Just to make sure we are on the same page. |
|
@hao-nan-li This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
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_saas_runtime_release" "primary" {
input_variable_defaults {
type = # value needed
}
input_variables {
type = # value needed
value = # value needed
variable = # value needed
}
output_variables {
type = # value needed
value = # value needed
variable = # value needed
}
}
Resource: resource "google_saas_runtime_unit_kind" "primary" {
default_release = # value needed
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
Multiple resources addedThis PR adds multiple new resources: |
Tests analyticsTotal tests: 6 Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
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_saas_runtime_release" "primary" {
input_variable_defaults {
type = # value needed
}
input_variables {
type = # value needed
value = # value needed
variable = # value needed
}
output_variables {
type = # value needed
value = # value needed
variable = # value needed
}
}
Resource: resource "google_saas_runtime_unit_kind" "primary" {
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
Multiple resources addedThis PR adds multiple new resources: |
Tests analyticsTotal tests: 6 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
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_saas_runtime_release" "primary" {
input_variables {
type = # value needed
value = # value needed
variable = # value needed
}
output_variables {
type = # value needed
value = # value needed
variable = # value needed
}
}
Resource: resource "google_saas_runtime_unit_kind" "primary" {
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
Multiple resources addedThis PR adds multiple new resources: |
Tests analyticsTotal tests: 6 Click here to see the affected service packages
Action takenFound 2 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: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
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_saas_runtime_unit_kind" "primary" {
output_variable_mappings {
to {
dependency = # value needed
ignore_for_lookup = # value needed
input_variable = # value needed
}
}
}
|
Tests analyticsTotal tests: 4 Click here to see the affected service packages
🟢 All tests passed! View the build log |
|
@GoogleCloudPlatform/terraform-team @hao-nan-li This PR has been waiting for review for 1 week. Please take a look! Use the label |
|
One thing I want to make sure before merging this: the added tests will run on a different project, do I need to re-do what we've done to create the blueprints on the other project? |
Yes, and actually I need to enable the API and create the P4SA in each testing project as well: https://yaqs.corp.google.com/eng/q/5003599241566748672 The commands needed before creating the blueprint are: |
commands applied on nightly beta test project |
|
@hao-nan-li This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
c02ebf9
Adds UnitKind resource provider for SaasRuntime API. (beta)