-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix inaccurate documentation for TargetHttpsProxy resource #10874
Fix inaccurate documentation for TargetHttpsProxy resource #10874
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @melinath, 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. |
@@ -134,7 +134,8 @@ properties: | |||
name: 'certificateManagerCertificates' | |||
description: | | |||
URLs to certificate manager certificate resources that are used to authenticate connections between users and the load balancer. | |||
Currently, you may specify up to 15 certificates. Certificate manager certificates do not apply when the load balancing scheme is set to INTERNAL_SELF_MANAGED. | |||
Currently, you may specify up to 15 certificates. Certificate manager certificates only apply when the load balancing scheme is set to INTERNAL_MANAGED. |
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.
"Currently, you may specify up to 15 certificates." this is not true for CertificateManagerCertificates.
I would suggest something like below. In line with sslCertificate definition.
URLs to Certificate manager certificate resources that are used to authenticate connections between users and the load balancer.
Certificate manager certificates only apply when the load balancing scheme is set to INTERNAL_MANAGED.
SslCertificates and CertificateManagerCertificates can not be defined together.
Accepted format is `//certificatemanager.googleapis.com/projects/{project}/locations/{location}/certificates/{resourceName}` or just the self_link `projects/{project}/locations/{location}/certificates/{resourceName}`
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.
Done! Just to confirm, the 15 certificate limit is still true for sslCertificates and not for certificate manager certificate, right?
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, exactly.
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 also update RegionTargetHttpsProxy - The 15 limit is not true and also regional INTERNAL_SELF_MANAGED doesn't exists so it can be dropped.
Suggestion:
URLs to Certificate manager certificate resources that are used to authenticate connections between users and the load balancer.
SslCertificates and CertificateManagerCertificates can not be defined together.
Accepted format is `//certificatemanager.googleapis.com/projects/{project}/locations/{location}/certificates/{resourceName}` or just the self_link `projects/{project}/locations/{location}/certificates/{resourceName}`
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.
Removing from my queue pending the changes suggested above; I will rely on y'all to let me know when these docs are accurate & then merge.
cc215a9
to
43e8e71
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.
|
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 testsTestAccComputeInstanceNetworkIntefaceWithSecurityPolicy |
|
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.
a couple minor nits based on the dev docs style guide.
@@ -134,7 +134,8 @@ properties: | |||
name: 'certificateManagerCertificates' | |||
description: | | |||
URLs to certificate manager certificate resources that are used to authenticate connections between users and the load balancer. | |||
Currently, you may specify up to 15 certificates. Certificate manager certificates do not apply when the load balancing scheme is set to INTERNAL_SELF_MANAGED. | |||
Certificate manager certificates only apply when the load balancing scheme is set to INTERNAL_MANAGED. | |||
For EXTERNAL and EXTERNAL_MANAGED please use certificate_map 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.
https://developers.google.com/style/word-list#please
For EXTERNAL and EXTERNAL_MANAGED please use certificate_map instead. | |
For EXTERNAL and EXTERNAL_MANAGED, use certificate_map 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.
Done
@@ -164,7 +165,8 @@ properties: | |||
name: 'certificateMap' | |||
description: | | |||
A reference to the CertificateMap resource uri that identifies a certificate map | |||
associated with the given target proxy. This field can only be set for global target proxies. | |||
associated with the given target proxy. This field is only supported for EXTERNAL and EXTERNAL_MANAGED load balancing schemes. | |||
For INTERNAL_MANAGED, please use certificate_manager_certificates 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.
For INTERNAL_MANAGED, please use certificate_manager_certificates instead. | |
For INTERNAL_MANAGED, use certificate_manager_certificates 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.
Done!
@@ -140,8 +140,7 @@ properties: | |||
name: 'certificateManagerCertificates' | |||
description: | | |||
URLs to certificate manager certificate resources that are used to authenticate connections between users and the load balancer. | |||
Currently, you may specify up to 15 certificates. Certificate manager certificates do not apply when the load balancing scheme is set to INTERNAL_SELF_MANAGED. | |||
sslCertificates and certificateManagerCertificates fields can not be defined together. | |||
sslCertificates and certificateManagerCertificates can not be defined together. |
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.
https://developers.google.com/style/contractions#negation-contractions
sslCertificates and certificateManagerCertificates can not be defined together. | |
sslCertificates and certificateManagerCertificates can't be defined together. |
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.
Done!
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: 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 testsTestAccComputeInstanceNetworkIntefaceWithSecurityPolicy |
|
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, going ahead with merge since @pawelJas has also approved
failing test is unrelated |
…udPlatform#10874) Co-authored-by: Hamza Hassan <hamzahassan@google.com>
…udPlatform#10874) Co-authored-by: Hamza Hassan <hamzahassan@google.com>
…udPlatform#10874) Co-authored-by: Hamza Hassan <hamzahassan@google.com>
This PR
google_compute_target_https_proxy
fails to provision withcertificate_manager_certificates
attribute hashicorp/terraform-provider-google#17176Release Note Template for Downstream PRs (will be copied)