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

AWS - Tag support for Elasticsearch #4973

Merged
merged 2 commits into from
Feb 17, 2016
Merged

AWS - Tag support for Elasticsearch #4973

merged 2 commits into from
Feb 17, 2016

Conversation

paultyng
Copy link
Contributor

@paultyng paultyng commented Feb 3, 2016

Since #3619 seems to have stalled, taking another stab at this. I'm very new to Go so I'm sure this isn't that idiomatic, but I've tried to copy the patterns for tags from other resources.

The weird thing with Elasticsearch's API is that it doesn't allow passing tags in a create, its always a separate call.

I still need to write the tests for the tags on the resource (I have tests written for the tagging methods though).

@paultyng
Copy link
Contributor Author

paultyng commented Feb 4, 2016

This now has testing for tags.

I need to run some additional runtime tests but its closer.

@paultyng paultyng changed the title [WIP] Tag support for Elasticsearch Tag support for Elasticsearch Feb 4, 2016
@paultyng paultyng changed the title Tag support for Elasticsearch AWS - Tag support for Elasticsearch Feb 4, 2016
@paultyng
Copy link
Contributor Author

paultyng commented Feb 4, 2016

Runtime testing on my template was successful, including just incremental tag update. This is ready for review.

@@ -37,13 +37,9 @@ func resourceAwsElasticSearchDomain() *schema.Resource {
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
if !regexp.MustCompile(`^[0-9A-Za-z]+`).MatchString(value) {
if !regexp.MustCompile(`^[a-z][0-9a-z\-]{2,27}$`).MatchString(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@paultyng why has this ValidateFunc changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are the actual rules for es domain names, I had it failing in the API call locally instead of in validation if I didn't meet those. Let me dig up the docs, one sec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://docs.aws.amazon.com/sdk-for-go/api/service/elasticsearchservice.html#type-CreateElasticsearchDomainInput

Domain names must start with a letter or number and can contain the following characters: a-z (lowercase), 0-9, and - (hyphen).

The text from the actual console is:

The name must start with a lowercase alphabet and be at least 3 and no more than 28 characters long. Valid characters are a-z (lowercase letters), 0-9, and - (hyphen).

And when I try to start with a number it doesn't work.

Feel free to cherry pick around this, or I can rip it out, I just hit this bug as well.

@stack72 stack72 self-assigned this Feb 16, 2016
@stack72
Copy link
Contributor

stack72 commented Feb 16, 2016

@paultyng this looks good - as soon as I can understand why the ValidateFunc changed :)

The tests look good as well

make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSElasticSearch_tags'
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
GO15VENDOREXPERIMENT=1 go generate $(GO15VENDOREXPERIMENT=1 go list ./... | grep -v /vendor/)
TF_ACC=1 GO15VENDOREXPERIMENT=1 go test ./builtin/providers/aws -v -run=AWSElasticSearch_tags -timeout 120m
=== RUN   TestAccAWSElasticSearch_tags
--- PASS: TestAccAWSElasticSearch_tags (1184.17s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    1184.192s

@paultyng
Copy link
Contributor Author

Added inline responses above. Basically bringing the client side validation in sync with the actual rules to fail faster on issues there.

@jen20
Copy link
Contributor

jen20 commented Feb 17, 2016

👍 for merging the fix for validation, but we should note this as a second CHANGELOG entry with the same PR number to avoid confusion.

@stack72
Copy link
Contributor

stack72 commented Feb 17, 2016

Thanks @paultyng - makes sense!

@jen20 will add the extra entry to the changelog

Paul

stack72 added a commit that referenced this pull request Feb 17, 2016
AWS - Tag support for Elasticsearch
@stack72 stack72 merged commit ec0e445 into hashicorp:master Feb 17, 2016
@paultyng paultyng deleted the pt/elasticsearch-tags branch February 17, 2016 14:10
@ghost
Copy link

ghost commented Apr 28, 2020

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
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.

3 participants