Skip to content
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

Merged
merged 7 commits into from
Nov 1, 2022

Conversation

Dudesons
Copy link
Contributor

@Dudesons Dudesons commented Aug 17, 2022

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

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

KUBE_CONFIG_PATH="~/.kube/config" make testacc TESTARGS='-run=TestAccKubernetesDataSourceEndpoints_basic'
==> Checking that code complies with gofmt requirements...
go vet .
TF_ACC=1 go test "/home/damien/go/src/github.com/hashicorp/terraform-provider-kubernetes/kubernetes" -v -run=TestAccKubernetesDataSourceEndpoints_basic -timeout 3h
=== RUN   TestAccKubernetesDataSourceEndpoints_basic
--- PASS: TestAccKubernetesDataSourceEndpoints_basic (8.73s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   8.764s

Release Note

Release note for CHANGELOG:

  * Add `kubernetes_endpoints_v1` data source

References

#1328

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Damien Goldenberg <[email protected]>
@hashicorp-cla
Copy link

hashicorp-cla commented Aug 17, 2022

CLA assistant check
All committers have signed the CLA.

@Dudesons
Copy link
Contributor Author

Dudesons commented Sep 5, 2022

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 ?

@Dudesons
Copy link
Contributor Author

Dudesons commented Oct 6, 2022

Hi, @BBBmau @jrhouston (I try with someone else)
There is something missing for reviewing this pull request ?

@alexsomesan
Copy link
Member

Sorry for the long wait, this somehow fell through the cracks.
We're looking into it now and will have feedback for you ASAP.

@arybolovlev
Copy link
Contributor

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:

  • Could you please add documentation for a new data source? It will be a new Markdown file in website/docs/d directory.
  • We recently made changes in the release process and now for a PR, we ask you to add a changelog file in .changelog/ directory. In your case, it will be a file .changelog/1805.txt. I will clarify the content of this file in my next comment.

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! 😊

Copy link
Contributor

@arybolovlev arybolovlev left a 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),
Copy link
Contributor

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" {
Copy link
Contributor

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.

@@ -213,6 +213,8 @@ func Provider() *schema.Provider {
"kubernetes_all_namespaces": dataSourceKubernetesAllNamespaces(),
"kubernetes_secret": dataSourceKubernetesSecret(),
"kubernetes_secret_v1": dataSourceKubernetesSecret(),
"kubernetes_endpoints": dataSourceKubernetesEndpoint(),
Copy link
Contributor

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),
Copy link
Contributor

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"
Copy link
Contributor

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"
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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) {
Copy link
Contributor

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
Copy link
Contributor

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.

@arybolovlev
Copy link
Contributor

For the changelog file, please use the tag feature, something like:

```release-note:feature
New data source: `kubernetes_endpoints_v1`
```

Looking forward to your changes!

Signed-off-by: Damien Goldenberg <[email protected]>
@Dudesons
Copy link
Contributor Author

Dudesons commented Oct 7, 2022

Hi, @arybolovlev ,

Thank you for your review, I did changes you're requested.
There are 2 parts where I still calling the kubernetes_endpoints because the function I'm using from the resource is still with the "old style":

I have a question, in the long term vision this provider wants to use resources with kind and apiVersion instead only the kind ?
I ask that because I'm using this provider since 3years now in production and having only the kind to manage was useful for version migration. Maybe one time we had a strange behavior when we had the hpa v2beta2 but not sure

@Dudesons
Copy link
Contributor Author

Dudesons commented Oct 7, 2022

I run test again on my local cluster:

 KUBE_CONFIG_PATH="~/.kube/config" make testacc TESTARGS='-run=TestAccKubernetesDataSourceEndpointsV1_basic'
==> Checking that code complies with gofmt requirements...
go vet .
TF_ACC=1 go test "/home/damien/go/src/github.com/hashicorp/terraform-provider-kubernetes/kubernetes" -v -run=TestAccKubernetesDataSourceEndpointsV1_basic -timeout 3h
=== RUN   TestAccKubernetesDataSourceEndpointsV1_basic
--- PASS: TestAccKubernetesDataSourceEndpointsV1_basic (6.22s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   6.251s

@Dudesons Dudesons requested a review from arybolovlev October 7, 2022 07:44
@arybolovlev
Copy link
Contributor

Thank you for your review, I did changes you're requested. There are 2 parts where I still calling the kubernetes_endpoints because the function I'm using from the resource is still with the "old style":

I have a question, in the long term vision this provider wants to use resources with kind and apiVersion instead only the kind ? I ask that because I'm using this provider since 3years now in production and having only the kind to manage was useful for version migration. Maybe one time we had a strange behavior when we had the hpa v2beta2 but not sure

Thank you! I am sorry, I was OOO and just came back.

No worries about resource kubernetes_endpoints, it is fine for now, we will fix that later.

To your question -- yes, that is right, but for backward compatibility, only kind resources have to stay. Although, they refer to the same function as kind + apiVersion.

Copy link
Contributor

@arybolovlev arybolovlev left a 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 {
Copy link
Contributor

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" {
Copy link
Contributor

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]>
@Dudesons
Copy link
Contributor Author

Hi @arybolovlev,

Sorry for the delay, I just did the update

@arybolovlev arybolovlev self-requested a review October 31, 2022 11:56
Copy link
Contributor

@arybolovlev arybolovlev left a 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! 👍🏻

Damien Goldenberg added 2 commits October 31, 2022 13:10
Signed-off-by: Damien Goldenberg <[email protected]>
Signed-off-by: Damien Goldenberg <[email protected]>
@arybolovlev arybolovlev merged commit 7469b44 into hashicorp:main Nov 1, 2022
@github-actions
Copy link

github-actions bot commented Dec 2, 2022

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants