-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for NFS and GCS mounts in Cloud Run v2 service #9728
Add support for NFS and GCS mounts in Cloud Run v2 service #9728
Conversation
```release-note:enhancement cloudrunv2: added `nfs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: added `gcs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: adding tcpSocket field to `google_cloud_run_v2.template.containers.livenessProbe` ```
Hello! I am a robot. It looks like you are a: Community Contributor @slevenick, 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. Terraform GA: Diff ( 3 files changed, 554 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_cloud_run_v2_service" "primary" {
template {
containers {
liveness_probe {
tcp_socket {
port = # value needed
}
}
}
volumes {
gcs {
read_only = # value needed
}
nfs {
read_only = # value needed
}
}
}
}
|
Hello @slevenick, @rileykarson, @melinath, and @ScottSuarez - firstly, wishing you a happy new year and sending my best regards. What @mvanholsteijn developed and mentioned in the content of this pull request is quite impressive, especially considering that this pull request was generated using that tool! I think it's worth checking what he has built and perhaps we could improve upon it :)! |
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 think that due to the auto-generated nature of a lot of these changes there are some invalid fields being added. Can you limit the changes to only those needed to support the feature in question? It will make testing and reviewing a lot easier
We have had some similar efforts in the past, and have always run into the same problem as is present in this PR. Discovery docs are not necessarily complete or consistent in how they represent field behavior. Discovery doc based generators tend to get the structure of the API right, but fail to accurately detect field behavior (due to no fault of the generator, the information just doesn't exist in the discovery docs!) which leads to a need for very thorough testing to make sure everything works. We've found that although a generator may help with the boilerplate of structuring the YAML it doesn't help out much in the long run as the difficulty in integrating a new feature or API tends to be in figuring out the actual behavior. They tend to be the most useful for the person who wrote the generator and has intimate knowledge of what it handles and doesn't handle, and which things they need to watch out for. Because of that it doesn't seem promising to encourage use of these sorts of generators for most contributors. |
94e8d41
to
7ccd199
Compare
Hi @slevenick , the change to the backendService accidentally was included in this pr. It happened in the second commit after processing the first review comments. It should not have been. My apologies. I have removed it. It is true that the scaffolder cannot be used without close inspection of the added properties, and it is not intended for as such. I also noted that there is another PR #9746 which implements support for the GCS volume mount only. Please advise on the sequencing of these PRs. |
I think we should favor this PR over #9746. Which version of the provider this feature should go in is not entirely clear, but I'd lean towards adding it to both beta & GA because it requires the user to specify the launch stage of BETA to use. |
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. Terraform GA: Diff ( 3 files changed, 558 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_cloud_run_v2_service" "primary" {
template {
containers {
liveness_probe {
tcp_socket {
port = # value needed
}
}
}
volumes {
gcs {
read_only = # value needed
}
}
}
}
|
Tests analyticsTotal tests: 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 testsTestAccCloudRunV2Service_cloudrunv2ServiceMountGcsExample|TestAccCloudRunV2Service_cloudrunv2ServiceMountNfsExample |
Rerun these tests in REPLAYING mode to catch issues
|
7ccd199
to
88ffe7f
Compare
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. Terraform GA: Diff ( 3 files changed, 558 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_cloud_run_v2_service" "primary" {
template {
containers {
liveness_probe {
tcp_socket {
port = # value needed
}
}
}
}
}
|
Tests analyticsTotal tests: 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 testsTestAccCloudRunV2Service_cloudrunv2ServiceMountGcsExample |
Rerun these tests in REPLAYING mode to catch issues
|
Thanks! |
…udPlatform#9728) * feat: add support for NFS and GCS mounts in Cloud Run v2 service ```release-note:enhancement cloudrunv2: added `nfs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: added `gcs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: adding tcpSocket field to `google_cloud_run_v2.template.containers.livenessProbe` ``` * fix: add required to relevant properties
…udPlatform#9728) * feat: add support for NFS and GCS mounts in Cloud Run v2 service ```release-note:enhancement cloudrunv2: added `nfs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: added `gcs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: adding tcpSocket field to `google_cloud_run_v2.template.containers.livenessProbe` ``` * fix: add required to relevant properties
Am I right that this PR only adds support for NFS and GCS for Cloud Run services, but not jobs? Is it feasible that as a total noob contributor I could create a PR to add support for jobs based off what was done in this PR? |
…udPlatform#9728) * feat: add support for NFS and GCS mounts in Cloud Run v2 service ```release-note:enhancement cloudrunv2: added `nfs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: added `gcs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: adding tcpSocket field to `google_cloud_run_v2.template.containers.livenessProbe` ``` * fix: add required to relevant properties
…udPlatform#9728) * feat: add support for NFS and GCS mounts in Cloud Run v2 service ```release-note:enhancement cloudrunv2: added `nfs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: added `gcs` field to `google_cloud_run_v2_service.template.volumes` cloudrunv2: adding tcpSocket field to `google_cloud_run_v2.template.containers.livenessProbe` ``` * fix: add required to relevant properties
Adds support for NFS and GCS mounts in Cloud Run v2 service. The volume definitions are already in the REST API definition.
I also included the tcpSocket in the liveness_probe, as that was missing too.
PS: Updates were made using the magic-module-scaffolder which automates the updates of Magic Module resource definitions.