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

ec_extension: implement extension resource #216

Merged
merged 25 commits into from
Jan 13, 2021

Conversation

snowhork
Copy link
Contributor

@snowhork snowhork commented Dec 25, 2020

This PR is for #102.
Our team want to manage extension with terraform.

Please review.

Description

This PR is new resource for extension.

How Has This Been Tested?

Execute commands below.

$ make install
$ cd examples/extension
$ terraform init
$ terraform apply

And then, I have checked the created extension.

Next, I have changed terraform file (changed description) and applied, then I have watched the extension description changed.

Next, I have removed ec_extension in terraform file and applied, then I have watched the extension removed.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improves code quality but has no user-facing effect)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Readiness Checklist

  • [x ] My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@snowhork snowhork requested a review from a team as a code owner December 25, 2020 22:55
@elasticmachine
Copy link
Collaborator

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

cla-checker-service bot commented Dec 25, 2020

💚 CLA has been signed

implement ec_extension resource and make example

Closes elastic#102
@snowhork
Copy link
Contributor Author

@marclop Could you review this PR?

@marclop marclop self-requested a review January 4, 2021 07:17
Copy link
Contributor

@marclop marclop 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 for your contribution, I appreciate the time you put in the development of this new resource.

I left a few comments on code structure and on the terraform flow.

Additionally there's two important things that are missing in this pull request before it can be merged; resource docs and acceptance tests, both are required before we can mark this as done.

The acceptance tests are underway ec/acc, three acceptance tests should be added:

  • Test 1 with the download_url set to a location where a file exists.
  • Test 2 with a bundle creation without any file_path and file_hash set. This test should a also test update changes (description, etc.)
  • Test 3 with a bundle creation with a file_path and file_hash set. This test should also test the behavior where file_hash changes and the file is re-uploaded.

Under the docs folder (docs/resources) a new ec_extension.md file should be added with a similar format to the other existing resources. It should describe what this resource does, its arguments with their default values and attributes.

Let me know if you need any help.

ec/ecresource/extensionresource/create.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/delete.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/read.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/schema.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/create.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/schema.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/update.go Outdated Show resolved Hide resolved
# Extension example

This example shows how to create an Elastic Cloud extension using Terraform.
You can create the extension by `files/content.json`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you meant in this sentence, it should be changed to something that guides the user how to create an actual extension, possibly to the Elasticsearch docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed by a55feb8. Please check this README?

@@ -0,0 +1,12 @@
# Extension example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to change this and the folder name to extension_bundle since it doesn't cover the plugin extension type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed by a55feb8. (Sorry, I can't check plugin type extension in my environment.)

@marclop marclop added the new-resource New resource requests label Jan 4, 2021
@snowhork
Copy link
Contributor Author

snowhork commented Jan 4, 2021

Thank you very much for your review. I'll check and fix.

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments on a few things I missed on the initial review.

ec/ecresource/extensionresource/delete.go Show resolved Hide resolved
ec/ecresource/extensionresource/read.go Show resolved Hide resolved
ec/ecresource/extensionresource/schema.go Show resolved Hide resolved
ec/ecresource/extensionresource/update.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/read_test.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/resource.go Outdated Show resolved Hide resolved
@snowhork
Copy link
Contributor Author

snowhork commented Jan 8, 2021

@marclop Thanks for your support. I'm writing acc test, and I have a question.

What should I set download_url ? Do you have any valid url for example?

docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/read.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/schema.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/update_test.go Outdated Show resolved Hide resolved
@marclop
Copy link
Contributor

marclop commented Jan 11, 2021

@snowhork I'm trying to get a download_url we can use in the acceptance tests that doesn't get accidentally removed, I'll let you know once I have that.

@marclop
Copy link
Contributor

marclop commented Jan 11, 2021

Jenkins test this please.

@marclop
Copy link
Contributor

marclop commented Jan 11, 2021

@snowhork you can use this link to obtain the analysis-icu plugin for the 7.10.1 version:

You could change it to any version that's published by changing the 7.10.1 suffix to another version:

https://artifacts.elastic.co/downloads/elasticsearch-plugins/analysis-icu/analysis-icu-$VERSION.zip

@snowhork
Copy link
Contributor Author

@marclop Thank you very much. I will fix soon.

@snowhork
Copy link
Contributor Author

@marclop I have added acc tests. please review.

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last nits, otherwise looks good.

ec/acc/testdata/extension_bundle_download.tf Outdated Show resolved Hide resolved
ec/acc/testdata/extension_bundle_download.tf Outdated Show resolved Hide resolved
ec/acc/extension_bundle_download_test.go Outdated Show resolved Hide resolved
ec/acc/extension_bundle_download_test.go Outdated Show resolved Hide resolved
@marclop
Copy link
Contributor

marclop commented Jan 11, 2021

@nrichers @alaudazzi can you have a look at the /docs files see if you folks would like anything to be changed before we merge it?

@karencfv do you want to give it a lass pass?

@marclop
Copy link
Contributor

marclop commented Jan 11, 2021

Jenkins test this please

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just left a few very minor comments, thanks for working on this @snowhork

docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
docs/resources/ec_extension.md Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/create.go Outdated Show resolved Hide resolved
ec/ecresource/extensionresource/read.go Show resolved Hide resolved
snowhork and others added 2 commits January 12, 2021 10:36
ec_extension: fix some comments

Co-authored-by: Karen Cárcamo <[email protected]>
@karencfv
Copy link
Contributor

Jenkins test this please

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-resource New resource requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants