-
Notifications
You must be signed in to change notification settings - Fork 987
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 the endpoints data source object #1805
Conversation
Signed-off-by: Damien Goldenberg <[email protected]>
Hi @alexsomesan , sorry to ping you but what is the process or what is missing in my PR to be reviewed and to unlock workflows ? |
Hi, @BBBmau @jrhouston (I try with someone else) |
Sorry for the long wait, this somehow fell through the cracks. |
Hi @Dudesons, Thank you for opening this PR and adding a new data source. There are a few things that are missed in this PR to be ready for merge:
I am still reviewing your code and it looks great in general, with just a few minor changes. I will come back to you with my comments soon. Thank you for your contribution! 😊 |
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 @Dudesons,
Thank you for your contribution. Looks good, just a few minor changes -- mostly renaming. For all new resources and data sources, we would like to have apiVersion
in their names. I might miss something and didn't comment everything, but hope you get the idea.
Again, nothing critical, your work looks great and I understand that you were looking back to the existing code but since this is a new resource let's use this chance to fix some previous mistakes. 😊
Please ping us once you address the comments or if you have any questions!
Thank you!
Config: testAccKubernetesEndpointsConfig_basic(name) + testAccKubernetesDataSourceEndpointsConfig_read(), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckKubernetesEndpointExists("kubernetes_endpoints.test", &conf), | ||
resource.TestCheckResourceAttr("kubernetes_endpoints.test", "metadata.0.name", name), |
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.
In the second step, we need to validate the data source resource, but for now, we validate the kubernetes_endpoints
resource.
Please, change the first argument here and below to data.kubernetes_endpoints_v1.test
.
} | ||
|
||
func testAccKubernetesDataSourceEndpointsConfig_read() string { | ||
return fmt.Sprintf(`data "kubernetes_endpoints" "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.
Please, use the data source name kubernetes_endpoints_v1
.
kubernetes/provider.go
Outdated
@@ -213,6 +213,8 @@ func Provider() *schema.Provider { | |||
"kubernetes_all_namespaces": dataSourceKubernetesAllNamespaces(), | |||
"kubernetes_secret": dataSourceKubernetesSecret(), | |||
"kubernetes_secret_v1": dataSourceKubernetesSecret(), | |||
"kubernetes_endpoints": dataSourceKubernetesEndpoint(), |
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 new resources and data sources, we use names with the API versions only. Please, remove kubernetes_endpoints
and leave only kubernetes_endpoints_v1
.
Config: testAccKubernetesEndpointsConfig_basic(name), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckKubernetesEndpointExists("kubernetes_endpoints.test", &conf), | ||
resource.TestCheckResourceAttr("kubernetes_endpoints.test", "metadata.0.name", name), |
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.
Could you please replace kubernetes_endpoints.test
and data.kubernetes_endpoints_v1.test
with variables? For example, as in this test file
|
||
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" | ||
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" | ||
api "k8s.io/api/core/v1" |
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 replace this import with corev1 "k8s.io/api/core/v1"
.
|
||
"github.com/hashicorp/terraform-plugin-sdk/v2/diag" | ||
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" | ||
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
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 replace this import with metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
.
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
func dataSourceKubernetesEndpoint() *schema.Resource { |
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 add a V1
suffix to the function name and make it plural to align with kind
and apiVersion
-- dataSourceKubernetesEndpointsV1
.
} | ||
} | ||
|
||
func dataSourceKubernetesEndpointRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { |
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 add a V1 suffix to the function name and make it plural to align with kind
and apiVersion
-- dataSourceKubernetesEndpointsV1Read
.
api "k8s.io/api/core/v1" | ||
) | ||
|
||
func TestAccKubernetesDataSourceEndpoints_basic(t *testing.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.
Please add a V1
suffix to the function name to align with kind
and apiVersion
-- TestAccKubernetesDataSourceEndpointsV1_basic.
@@ -0,0 +1,35 @@ | |||
package kubernetes |
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 rename this file with the apiVersion
suffix -- data_source_kubernetes_endpoints_v1.go
and the same for the related test file.
For the changelog file, please use the tag
Looking forward to your changes! |
Signed-off-by: Damien Goldenberg <[email protected]>
Hi, @arybolovlev , Thank you for your review, I did changes you're requested.
I have a question, in the long term vision this provider wants to use resources with |
I run test again on my local cluster:
|
Thank you! I am sorry, I was OOO and just came back. No worries about resource To your question -- yes, that is right, but for backward compatibility, only |
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 @Dudesons,
Looks great! Thanks a lot, just a small rename fix in the doc and function name. Once it is done we are good to merge and a new data source will be available in the upcoming release.
Thanks! 🚀👍🏻
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
func dataSourceKubernetesEndpointV1() *schema.Resource { |
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.
Could you please rename dataSourceKubernetesEndpointV1
to dataSourceKubernetesEndpointsV1
?
## Example Usage | ||
|
||
```hcl | ||
data "kubernetes_endpoints" "api_endpoints" { |
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.
Could you please change data source name from kubernetes_endpoints
to kubernetes_endpoints_v1
?
Signed-off-by: Damien Goldenberg <[email protected]>
Hi @arybolovlev, Sorry for the delay, I just did the update |
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.
Thank you @Dudesons! 🚀 Great job! 👍🏻
Signed-off-by: Damien Goldenberg <[email protected]>
Signed-off-by: Damien Goldenberg <[email protected]>
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Signed-off-by: Damien Goldenberg [email protected]
Description
As a Terraform user in some cases I need to read endpoints objects.
An example is: I need to set a network policy on an workload and this one need to request the Kubernetes api. In this case the cluster IP doesn't work I need IPs from the endpoints object
Acceptance tests
Output from acceptance testing:
Release Note
Release note for CHANGELOG:
References
#1328
Community Note