-
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
Add support for PrivateRegistryAccessConfig #10591
Add support for PrivateRegistryAccessConfig #10591
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SarahFrench, 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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_container_cluster" "primary" {
node_config {
containerd_config {
private_registry_access_config {
certificate_authority_domain_config {
fqdns = # value needed
gcp_secret_manager_certificate_config {
secret_uri = # value needed
}
}
enabled = # value needed
}
}
}
node_pool {
node_config {
containerd_config {
private_registry_access_config {
certificate_authority_domain_config {
fqdns = # value needed
gcp_secret_manager_certificate_config {
secret_uri = # value needed
}
}
enabled = # 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 testsTestAccContainerCluster_privateRegistry|TestAccContainerNodePool_privateRegistry |
|
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.
Hi, thanks for your PR!
Could you please add additional tests that cover the fields mentioned in this message? Your new tests cover fields nested under node_pool_defaults
but that message indicates they can also be set under node_config and node_pool top level fields too.
Also, do you have any API documentation you could share about these new API fields? Thanks!
mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.erb
Outdated
Show resolved
Hide resolved
274329c
to
a2a95d4
Compare
Added acceptance 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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_container_cluster" "primary" {
node_config {
containerd_config {
private_registry_access_config {
certificate_authority_domain_config {
fqdns = # value needed
gcp_secret_manager_certificate_config {
secret_uri = # value needed
}
}
enabled = # value needed
}
}
}
node_pool {
node_config {
containerd_config {
private_registry_access_config {
certificate_authority_domain_config {
fqdns = # value needed
gcp_secret_manager_certificate_config {
secret_uri = # value needed
}
}
enabled = # value needed
}
}
}
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
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.
As well as our thread on provisioning the secret that's referenced in the acceptance tests, please follow the guidance in this automated message: #10591 (comment)
4959947
to
d42debf
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_container_cluster" "primary" {
node_config {
containerd_config {
private_registry_access_config {
certificate_authority_domain_config {
fqdns = # value needed
gcp_secret_manager_certificate_config {
secret_uri = # value needed
}
}
enabled = # 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 testsTestAccContainerCluster_privateRegistry|TestAccContainerNodePool_privateRegistry |
|
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.
Left some comments about the config for the added secrets - we need to make sure the names are unique, which is done by passing in a random string to the config. Here's an example:
Line 23 in 8ad7f68
policyTitle := acctest.RandString(t, 10) |
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb
Outdated
Show resolved
Hide resolved
Thanks for the detailed review of this PR. Added random string to the secret name/ID. |
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 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_privateRegistry|TestAccContainerNodePool_privateRegistry |
|
Thanks for making the changes I asked, merging now 🚀 |
Co-authored-by: Mike Miranda <mkmir@google.com>
Co-authored-by: Mike Miranda <mkmir@google.com>
Co-authored-by: Mike Miranda <mkmir@google.com>
Co-authored-by: Mike Miranda <mkmir@google.com>
Co-authored-by: Mike Miranda <mkmir@google.com>
Co-authored-by: Mike Miranda <mkmir@google.com>
This PR adds support for PrivateRegistryAccessConfig. See the public docs:
Release Note Template for Downstream PRs (will be copied)
PS: This PR is the clone of the work authored by @mmiranda96 in this PR: #10450. Thanks Mike!