Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Cloud Deploy resources #5710
Add support for Cloud Deploy resources #5710
Changes from 4 commits
2b34526
617daea
c67424c
f53e541
d8e431f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These are unnecessary as the whole resource is input: 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.
Is this needed for any particular reason? We tend to avoid adding
etag
and some of these other output-only fields if they aren't needed for other resources that depend on this. Similarly, updateTime & createTime can be removedThere 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.
Were these needed?
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
name
the right format? This is the short-form, so won't be able to identify the full resource during import?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
https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-5710-old..auto-pr-5710#diff-e36b98e511d7d496a126e50204560eea400fe43213204672a22acece89f3f4ebR409
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.
On a side note, I'm not a fan of something_Id as name is the consistent resource name in most resources. The API uses name as selflink but an identical value can be read with the id attribute.
Precedent: #4951
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, having this id_format ends up with the following code to set the ID of the resource:
I don't believe this is right, because it doesn't represent the full resource. The ID format should be
projects/{{project}}/locations/{{region}}/deliveryPipelines/{{name}}
so that we can reference this value from other resources within a config (if needed)