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
redevelop the VPC module upgrade to Terraform 0.12 #28
base: master
Are you sure you want to change the base?
redevelop the VPC module upgrade to Terraform 0.12 #28
Changes from 11 commits
3098fd5
d1b034d
a792e1e
48bfdd6
72d1865
d699b48
3e9b3b3
038e33f
82b7cdf
ac16eb3
68c62cb
d031e88
482681b
677e174
336c598
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.
I think
this_vpc_id
is ok and why change it tovpc_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.
it is to be consistent with the other outputs of vpc
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.
The other outputs of vpc also have a prefix
this_
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 not, I think them should be added.
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.
Please improve the outputs and set a prefix
this_
for them.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 am a bit lost on the advantage to use "this". When I saw other Terraform modules in GitHub, the maintainer omits the name of the resource, in this case "this" because it is unique.
You can take as example these other alibaba modules:
I propose I add the "this" and I keep also the other ones.
As for the description, the "The VPC id" is the actual description of today's module. How do you suggest to improve ?
"The VPC id created by the Terraform module" ?
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 "The VPC id created by the Terraform module." is ok. But, I think there is no need to keep both. Other modules' outputs also need to be improved. The module https://github.com/terraform-alicloud-modules/terraform-alicloud-kubernetes/blob/master/outputs.tf has not improved yet. But most modules have had
this
prefix for each output.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 not worked on the alicloud kubernetes, no PRs from my side at the moment. I looked at other official terraform modules and "this" or other identifiers are not used when there is only one type of resource.
I think we should follow this current best practice as it is also easier to read
In any case, I removed the duplicates ,)
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 nat gateway only has one snat table. Please use
nat_gateway_snat_table_id
instead.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 it should be corrected also in the attributes of the resource because it returns
snat_table_ids
https://registry.terraform.io/providers/aliyun/alicloud/latest/docs/resources/nat_gateway#attributes-reference
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 nat gateway only has one dnat table. Please use nat_gateway_dnat_table_id instead.
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 it should be corrected also in the attributes of the resource because it returns
forward_table_ids
https://registry.terraform.io/providers/aliyun/alicloud/latest/docs/resources/nat_gateway#attributes-reference
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.
Ok, will do. Thanks.