-
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 GPU fields in Cloud Run Service v1 and v2 api #11597
Conversation
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.
|
Tests analyticsTotal tests: 74 Click here to see the affected service packages
Tests were added that are skipped in VCR:
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
|
|
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.
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @shuyama1, 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. |
Tests analyticsTotal tests: 74 Click here to see the affected service packages
Tests were added that are skipped in VCR:
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
|
|
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.
|
Tests analyticsTotal tests: 74 Click here to see the affected service packages
Tests were added that are skipped in VCR:
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
|
|
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.
|
Tests analyticsTotal tests: 74 Click here to see the affected service packages
Tests were added that are skipped in VCR:
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
|
|
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.
|
Tests analyticsTotal tests: 30 Click here to see the affected service packages
Tests were added that are skipped in VCR:
View the build log |
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.
|
Tests analyticsTotal tests: 76 Click here to see the affected service packages
Tests were added that are skipped in VCR:
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
|
|
description: Node Selector describes the hardware requirements of the resources. | ||
properties: | ||
- !ruby/object:Api::Type::String | ||
name: 'accelerator' |
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.
Can we make this field required, given this is the only subfield under node_selector
currently. We'd generally want to prevent users sending empty blocks.
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.
If different fields are added later and all are optional, is it a breaking change to remove required on accelerator
?
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.
Nope, required -> optional is non-breaking
<% unless version == 'ga' -%> | ||
|
||
func TestAccCloudRunService_resourcesRequirements(t *testing.T) { | ||
acctest.SkipIfVcr(t) |
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.
Is there any particular reason that we need to skip VCR for this test?
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.
Thanks for catching. I have removed these tests in favor of the examples.
@@ -0,0 +1,34 @@ | |||
resource "google_cloud_run_service" "<%= ctx[:primary_resource_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.
We'll need to add an example block to the resource.yaml file to register this example for documentation and test case generation. Please refer to step 5 in https://googlecloudplatform.github.io/magic-modules/develop/test/test/#add-a-create-test for guidance. Additionally, we can remove the handwritten test since the resource configurations in the examples and the handwritten test are mostly identical.
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.
Thanks. I have removed the tests in favor of the examples.
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.
|
Tests analyticsTotal tests: 79 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
|
|
Howdy! I love this. Is this ready for review and merging? Happy to help if anything else is needed. |
@shuyama1 This PR has been waiting for review for 3 weekdays. 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.
LGTM overall, only a small comment regarding the example version
Thanks! I have applied the suggested fixes. |
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.
|
Tests analyticsTotal tests: 79 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
|
|
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 follow step 3 in https://googlecloudplatform.github.io/magic-modules/develop/test/test/#add-a-create-test to add provider = google-beta
to every resource in your example configuration file. This should resolve the test failure. Sorry that I pointed to the wrong step in my previous comment.
Co-authored-by: Shuya Ma <[email protected]>
Co-authored-by: Shuya Ma <[email protected]>
Ah, thank you. I have updated the example files. |
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.
|
Tests analyticsTotal tests: 79 Click here to see the affected service packages
View the build log |
Adds support for GPU fields which requires changes to limits as well as adding a new field
nodeSelector
to v1 and v2 api.Release Note Template for Downstream PRs (will be copied)