Add script resource #173

k-yomo · 2022-10-29T19:36:50Z

Resolves #107

This PR adds script resource in reference to the below docs.
https://www.elastic.co/guide/en/elasticsearch/reference/current/create-stored-script-api.html
https://www.elastic.co/guide/en/elasticsearch/reference/current/get-stored-script-api.html

📝 context and params can't be retrieved by GET endpoint, so they are not importable as of now.

elasticmachine · 2022-10-29T19:36:52Z

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?

tobio · 2022-10-30T10:28:12Z

internal/clients/cluster.go

+	type getScriptResponse = struct {
+		Script *models.Script `json:"script"`
+	}
+	var scriptResponse getScriptResponse


Suggested change

type getScriptResponse = struct {

Script *models.Script `json:"script"`

}

var scriptResponse getScriptResponse

var scriptResponse struct {

Script *models.Script `json:"script"`

}

Any benefit to the named type here?

Oh no, unnamed type would be fine, fixed so!

tobio · 2022-10-30T10:29:19Z

internal/clients/cluster.go

+	type getScriptResponse = struct {
+		Script *models.Script `json:"script"`
+	}
+	var scriptResponse getScriptResponse


Suggested change

type getScriptResponse = struct {

Script *models.Script `json:"script"`

}

var scriptResponse getScriptResponse

var scriptResponse struct {

Script *models.Script `json:"script"`

}

Any benefit to the named type here?

tobio · 2022-10-30T22:25:27Z

docs/resources/elasticsearch_script.md

+---
+# generated by https://github.com/hashicorp/terraform-plugin-docs
+page_title: "elasticstack_elasticsearch_script Resource - terraform-provider-elasticstack"
+subcategory: ""


subcategory: "Cluster" (I think, not sure if this warrants it's own category)

Might need to add a template

Oh totally missed it, will fix.
I want this😂
hashicorp/terraform-plugin-docs#156

Nice, yep very much want that one too :)

tobio · 2022-10-30T22:29:56Z

internal/clients/cluster.go

+	req := struct {
+		Script *models.Script `json:"script"`
+	}{
+		Script: script,


Suggested change

Script: script,

script,

tobio · 2022-10-30T22:31:01Z

internal/clients/cluster.go

+	if res.StatusCode == http.StatusNotFound {
+		return nil, nil
+	}
+	if diags := utils.CheckError(res, fmt.Sprintf("Unable to get the script: %s", id)); diags.HasError() {


Suggested change

if diags := utils.CheckError(res, fmt.Sprintf("Unable to get the script: %s", id)); diags.HasError() {

if diags := utils.CheckError(res, fmt.Sprintf("Unable to get saved script: %s", id)); diags.HasError() {

tobio · 2022-10-30T22:31:27Z

internal/clients/cluster.go

+		return diag.FromErr(err)
+	}
+	defer res.Body.Close()
+	if diags := utils.CheckError(res, "Unable to put the script"); diags.HasError() {


Suggested change

if diags := utils.CheckError(res, "Unable to put the script"); diags.HasError() {

if diags := utils.CheckError(res, "Unable to put saved script"); diags.HasError() {

tobio · 2022-10-30T22:32:48Z

internal/elasticsearch/cluster/script.go

+			Description:  "Script language. For search templates, use `mustache`. Defaults to `painless`.",
+			Type:         schema.TypeString,
+			Optional:     true,
+			Default:      "painless",


This default makes sense in the context of a stored script, but not when used to manage a search template. IMO it would be better to mirror the API and have this as a required field without a default.

Oh that's true, fixed to make it required field.

tobio · 2022-10-30T22:35:31Z

internal/elasticsearch/cluster/script.go

+
+	script, diags := client.GetElasticsearchScript(ctx, compId.ResourceId)
+	if !d.IsNewResource() && script == nil && diags == nil {
+		tflog.Warn(ctx, fmt.Sprintf(`Script "%s" not found, removing from state`, compId.ResourceId))


It doesn't look like we log a message in this case for the other resources. Any reason this should be a warning?

Yes, the reason is to let the user know that the resource is removed from state when actually not found on API which is sometimes unexpected for users.
I think it's pretty common pattern which the other providers do too.
https://github.com/hashicorp/terraform-provider-aws/search?q=removing+from+state

It it makes sense, I'll add it to the other resources too with !d.IsNewResource() check.
hashicorp/terraform-provider-aws#16796

I just realized that adding !d.IsNewResource() is not good here since it can cause null pointer error in the following code.
It might be better to return error when the resource is d.IsNewResource()) as aws provider does, but I simply removed to align with the other resources for now.

tobio · 2022-10-30T22:36:48Z

internal/elasticsearch/cluster/script_test.go

+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+)
+
+func TestAccResourceScript(t *testing.T) {


Can we add a test managing a search template as well?

tobio

Couple of minor docs changes.

tobio · 2022-10-31T20:56:48Z

docs/resources/elasticsearch_script.md

+resource "elasticstack_elasticsearch_script" "my_script" {
+  script_id = "my_script"
+  source    = "Math.log(_score * 2) + params['my_modifier']"
+  context   = "score"


This example is no longer valid, and needs lang defined.

Oh good catch, fixed🙏
5bde0d4

tobio · 2022-10-31T20:59:52Z

docs/resources/elasticsearch_script.md

+
+Creates or updates a stored script or search template. See https://www.elastic.co/guide/en/elasticsearch/reference/current/create-stored-script-api.html
+
+## Example Usage


Should have said this earlier sorry, having a search template example would be nice too.

k-yomo force-pushed the add-script-resource branch 6 times, most recently from 8f29b85 to d11a0ca Compare October 30, 2022 05:21

Implement script resource

c76a508

k-yomo force-pushed the add-script-resource branch from d11a0ca to 3f4af9f Compare October 30, 2022 05:35

Generate docs for script resource

d027f42

k-yomo force-pushed the add-script-resource branch from 3f4af9f to d027f42 Compare October 30, 2022 05:35

k-yomo marked this pull request as ready for review October 30, 2022 05:41

Update CHANGELOG

f21d27b

tobio reviewed Oct 30, 2022

View reviewed changes

k-yomo added 3 commits October 31, 2022 18:58

Fixed reviewed ones

3633a63

Merge branch 'main' into add-script-resource

3c1c0f6

Fix test

77fa79f

k-yomo requested a review from tobio October 31, 2022 10:07

Fix subcategory for elasticstack_elasticsearch_script

50b83bd

tobio reviewed Oct 31, 2022

View reviewed changes

Fix elasticstack_elasticsearch_script docs

5bde0d4

k-yomo requested a review from tobio November 1, 2022 02:03

k-yomo and others added 4 commits November 1, 2022 11:05

Format code

b5327c1

Regenerate docs

24a6461

Remove new resource check

f62fd11

Merge branch 'main' into add-script-resource

7d16960

tobio approved these changes Nov 1, 2022

View reviewed changes

tobio merged commit 1131f2a into elastic:main Nov 1, 2022

k-yomo deleted the add-script-resource branch November 2, 2022 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script resource #173

Add script resource #173

k-yomo commented Oct 29, 2022 •

edited

Loading

elasticmachine commented Oct 29, 2022

tobio Oct 30, 2022

k-yomo Oct 31, 2022 •

edited

Loading

tobio Oct 30, 2022

tobio Oct 30, 2022

k-yomo Oct 31, 2022

tobio Oct 31, 2022

tobio Oct 30, 2022

tobio Oct 30, 2022

tobio Oct 30, 2022

tobio Oct 30, 2022

k-yomo Oct 31, 2022

tobio Oct 30, 2022

k-yomo Oct 31, 2022 •

edited

Loading

k-yomo Nov 1, 2022 •

edited

Loading

tobio Oct 30, 2022

tobio left a comment

tobio Oct 31, 2022

k-yomo Nov 1, 2022 •

edited

Loading

tobio Oct 31, 2022

	if diags := utils.CheckError(res, fmt.Sprintf("Unable to get the script: %s", id)); diags.HasError() {
	if diags := utils.CheckError(res, fmt.Sprintf("Unable to get saved script: %s", id)); diags.HasError() {

	if diags := utils.CheckError(res, "Unable to put the script"); diags.HasError() {
	if diags := utils.CheckError(res, "Unable to put saved script"); diags.HasError() {


		Creates or updates a stored script or search template. See https://www.elastic.co/guide/en/elasticsearch/reference/current/create-stored-script-api.html

		## Example Usage

Add script resource #173

Add script resource #173

Conversation

k-yomo commented Oct 29, 2022 • edited Loading

elasticmachine commented Oct 29, 2022

Choose a reason for hiding this comment

k-yomo Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-yomo Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

k-yomo Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-yomo Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-yomo commented Oct 29, 2022 •

edited

Loading

k-yomo Oct 31, 2022 •

edited

Loading

k-yomo Oct 31, 2022 •

edited

Loading

k-yomo Nov 1, 2022 •

edited

Loading

k-yomo Nov 1, 2022 •

edited

Loading