Add component template resource #39

xunleii · 2022-01-20T22:57:49Z

This PR adds new component_template resource to manage components template through TF resources.
This is a "naive" implementation of this resource, mainly based on the index_template one (many part are copy/paste).

Of course, if needed, I can spend more time to see how can I simplify by sharing code between index_template and component_template, but could decrease the readability.

NOTE: this PR is "in conflict" with the https://github.com./elastic/terraform-provider-elasticstack/pull/38/files on how we flatten struct; I use a method directly on models and @olksdr uses a function that take the object as parameter. Because its PRs has been already reviewed, I can adapt my code for its PR to avoid conflict (keeping its work or using method on models, whichever you and @olksdr prefer).
NOTE²: https://github.com./elastic/terraform-provider-elasticstack/blob/main/internal/elasticsearch/index/template.go#L220 will panic if no aliases are defined on index_template. In order to avoid any conflict with https://github.com./elastic/terraform-provider-elasticstack/pull/38/files, I didn't made any changes to fix that. But if my PR is be based on the later one, I will fix it.
NOTE³: My changes need more tests, so I'll add them tomorrow

elasticmachine · 2022-01-20T22:57:51Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cla-checker-service · 2022-01-20T22:57:54Z

💚 CLA has been signed

olksdr · 2022-01-21T06:27:23Z

Hey @xunleii! Thanks for your contribution!

NOTE²: https://github.com./elastic/terraform-provider-elasticstack/blob/main/internal/elasticsearch/index/template.go#L220 will panic if no aliases are defined on index_template.

Could you, please, also provide the error you get? It would be great if you could open an issue with error and possibly traces, your TF version, the resource definition you're using to test this and system you're running this on.

xunleii · 2022-01-21T08:18:29Z

Hi @olksdr, thanks for your response (and thanks to the team for this provider).
For the panic, I probably missed something when I ran the acceptance tests yesterday; I can no longer reproduce the panic on index_template on a clean environment (nor on component_template if I delete the last commit)... so should probably be a PEBCAK 😅
~~Also, is there anything to do to validate the CLA verification? I signed the agreement yesterday but this step remains failed (probably missed something).~~

olksdr · 2022-02-02T07:36:59Z

Hey @xunleii ,
could you, please, merge the main in and resolve the conflicts in your branch? So we could review your changes.

NOTE: this PR is "in conflict" with the https://github.com./elastic/terraform-provider-elasticstack/pull/38/files on how we flatten struct; I use a method directly on models and @olksdr uses a function that take the object as parameter. Because its PRs has been already reviewed, I can adapt my code for its PR to avoid conflict (keeping its work or using method on models, whichever you and @olksdr prefer).

I would prefer to use existing function, which is already used in the index_template and index resources. But proposed changes also make sense.

It would be better to keep this PR simpler and just introduce the new resource, with tests and docs and move any refactoring into separate PR. With refactoring it would be great to move many of free standing functions into the models methods.

xunleii · 2022-02-03T18:42:45Z

Hi @olksdr,

I rebased my branch on main and removed all changes not related to the main subject (adding the component_template).
If I have some free time, I will try to make the PR about refactoring (but I a bit busy this time).

Thanks for your review !

olksdr

Looks nice!

I've added few comments about documentation generation, and the placement of the new resource.

olksdr · 2022-02-04T09:19:43Z

docs/resources/elasticsearch_component_template.md

@@ -0,0 +1,101 @@
+---


All the documentation in the docs/ folder is generated and should not be changed manually. See how it's done for template resource:

terraform-provider-elasticstack/templates/resources/elasticsearch_index_template.md.tmpl

Lines 1 to 12 in 0beb7d4

---

subcategory: "Index"

layout: ""

page_title: "Elasticstack: elasticstack_elasticsearch_index_template Resource"

description: |-

Creates or updates an index template.

---

# Resource: elasticstack_elasticsearch_index_template

Creates or updates an index template. Index templates define settings, mappings, and aliases that can be applied automatically to new indices. See, https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-put-template.html

you could copy this template and change the specific parts in names and the paths to the examples and run make gen to generate the required files, which you then can add into this PR as well.

I used make docs-generate to generate this file, but without adding the template file. I'll add it and regenete it the new documentation just after

olksdr · 2022-02-04T09:21:51Z

internal/elasticsearch/cluster/component_template.go

@@ -0,0 +1,290 @@
+package cluster


Could you, please, move this file into internal/elasticsearch/index/ folder (index module), where the template resource is located, just to keep them all together?

olksdr · 2022-02-04T09:23:52Z

internal/elasticsearch/cluster/component_template.go

+
+func ResourceComponentTemplate() *schema.Resource {
+	// NOTE: component_template and index_template uses the same schema
+	componentTemplateSchema := map[string]*schema.Schema{


Please, also add the ID as Computed value:

Suggested change

componentTemplateSchema := map[string]*schema.Schema{

componentTemplateSchema := map[string]*schema.Schema{

"id": {

Description: "Internal identifier of the resource",

Type: schema.TypeString,

Computed: true,

},

olksdr · 2022-02-04T09:27:23Z

ok to test

olksdr · 2022-02-04T09:29:13Z

And one more thing:
please add the record in CHANGELOG.md, like here

terraform-provider-elasticstack/CHANGELOG.md

Line 3 in 0beb7d4

    
           - New resource `elasticstack_elasticsearch_data_stream` to manage Elasticsearch [data streams](https://www.elastic.co/guide/en/elasticsearch/reference/current/data-streams.html) ([#45](https://github.com./elastic/terraform-provider-elasticstack/pull/45))

xunleii · 2022-02-05T17:02:27Z

Hi @olksdr, thanks for your review. I made all changes that you request me and put them inside two fixup commits if you want to check these changes only.
When all will be good, I will squash them to clean the git history.

olksdr

Thanks for addressing my review comments.

Only one small thing left to do, see my 2 proposed suggestion, to handle the situation when the resource was deleted outside of the TF. Those 2 pieces of. code also need to be added in, and we are good to merge this new resource.

And thanks again for working on this!

internal/clients/index.go

internal/elasticsearch/index/component_template.go

Co-authored-by: Oleksandr <[email protected]>

xunleii · 2022-02-10T08:00:22Z

Hi, I applied your suggestion and resolved the conflict on CHANGELOG. Thanks

olksdr

lgtm

olksdr · 2022-02-10T09:38:26Z

jenkins test this

olksdr added the enhancement New feature or request label Jan 21, 2022

xunleii force-pushed the main branch from 174c065 to bd4786b Compare January 21, 2022 08:22

olksdr added the Elasticsearch Elasticsearch related APIs label Feb 3, 2022

xunleii force-pushed the main branch 2 times, most recently from bfc4ef2 to 0beb7d4 Compare February 3, 2022 18:35

xunleii marked this pull request as ready for review February 3, 2022 18:42

xunleii requested a review from olksdr as a code owner February 3, 2022 18:42

olksdr requested a review from Crazybus February 4, 2022 05:14

olksdr self-assigned this Feb 4, 2022

olksdr suggested changes Feb 4, 2022

View reviewed changes

xunleii requested a review from olksdr February 5, 2022 17:03

olksdr suggested changes Feb 7, 2022

View reviewed changes

internal/clients/index.go Show resolved Hide resolved

internal/elasticsearch/index/component_template.go Show resolved Hide resolved

xunleii force-pushed the main branch from 18e6155 to bf2f3e9 Compare February 10, 2022 07:56

Alexandre Nicolaie dit Clairville and others added 4 commits February 10, 2022 08:58

Implement naively component template

82d1dfc

Add example and update docs

de15830

Update CHANGELOG: support component template resources

3e6ca3e

Apply GH review suggestion

dc34ed0

Co-authored-by: Oleksandr <[email protected]>

xunleii force-pushed the main branch from bf2f3e9 to dc34ed0 Compare February 10, 2022 07:58

olksdr approved these changes Feb 10, 2022

View reviewed changes

olksdr merged commit 025037a into elastic:main Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add component template resource #39

Add component template resource #39

xunleii commented Jan 20, 2022 •

edited

Loading

elasticmachine commented Jan 20, 2022

cla-checker-service bot commented Jan 20, 2022 •

edited

Loading

olksdr commented Jan 21, 2022

xunleii commented Jan 21, 2022 •

edited

Loading

olksdr commented Feb 2, 2022

xunleii commented Feb 3, 2022

olksdr left a comment

olksdr Feb 4, 2022

xunleii Feb 5, 2022

olksdr Feb 4, 2022

olksdr Feb 4, 2022

olksdr commented Feb 4, 2022

olksdr commented Feb 4, 2022

xunleii commented Feb 5, 2022 •

edited

Loading

olksdr left a comment

xunleii commented Feb 10, 2022

olksdr left a comment

olksdr commented Feb 10, 2022

	---
	subcategory: "Index"
	layout: ""
	page_title: "Elasticstack: elasticstack_elasticsearch_index_template Resource"
	description: \|-
	Creates or updates an index template.
	---

	# Resource: elasticstack_elasticsearch_index_template

	Creates or updates an index template. Index templates define settings, mappings, and aliases that can be applied automatically to new indices. See, https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-put-template.html

-	componentTemplateSchema := map[string]*schema.Schema{
+    componentTemplateSchema := map[string]*schema.Schema{
+			"id": {
+			Description: "Internal identifier of the resource",
+			Type:        schema.TypeString,
+			Computed:    true,
+		},

Add component template resource #39

Add component template resource #39

Conversation

xunleii commented Jan 20, 2022 • edited Loading

elasticmachine commented Jan 20, 2022

cla-checker-service bot commented Jan 20, 2022 • edited Loading

olksdr commented Jan 21, 2022

xunleii commented Jan 21, 2022 • edited Loading

olksdr commented Feb 2, 2022

xunleii commented Feb 3, 2022

olksdr left a comment

Choose a reason for hiding this comment

olksdr Feb 4, 2022

Choose a reason for hiding this comment

xunleii Feb 5, 2022

Choose a reason for hiding this comment

olksdr Feb 4, 2022

Choose a reason for hiding this comment

olksdr Feb 4, 2022

Choose a reason for hiding this comment

olksdr commented Feb 4, 2022

olksdr commented Feb 4, 2022

xunleii commented Feb 5, 2022 • edited Loading

olksdr left a comment

Choose a reason for hiding this comment

xunleii commented Feb 10, 2022

olksdr left a comment

Choose a reason for hiding this comment

olksdr commented Feb 10, 2022

xunleii commented Jan 20, 2022 •

edited

Loading

cla-checker-service bot commented Jan 20, 2022 •

edited

Loading

xunleii commented Jan 21, 2022 •

edited

Loading

xunleii commented Feb 5, 2022 •

edited

Loading