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.

Sign up for GitHub

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

Jump to bottom

resource/aws_cloudwatch_log_metric_filter: Add support for DefaultValue #1578

Merged
merged 2 commits into from
Oct 30, 2017
Merged

resource/aws_cloudwatch_log_metric_filter: Add support for DefaultValue #1578

merged 2 commits into from
Oct 30, 2017

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 3, 2017 •
edited by Ninir
Loading

Fixes: #1563
Fixes: #1442

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudWatchLogMetricFilter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudWatchLogMetricFilter_ -timeout 120m
=== RUN   TestAccAWSCloudWatchLogMetricFilter_basic
--- PASS: TestAccAWSCloudWatchLogMetricFilter_basic (59.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	59.047s

@stack72 stack72 mentioned this pull request Sep 3, 2017
Closed
@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 6, 2017
Ninir
Ninir reviewed Sep 8, 2017
View reviewed changes
aws/structure.go Outdated
f, err := strconv.ParseFloat(bound, 64)
if err != nil {
return nil, fmt.Errorf(
"default_value must be a float value represented as a string")
Copy link
Contributor

@Ninir Ninir Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think it would be worth to make a validation function for that?
We could get the info early at plan time. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ninir I changed to use the built in schema.TypeFloat to avoid the need for parsing

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 8, 2017
@stack72 stack72 force-pushed the b-aws-logs-filter-1563 branch from 642fdbd to 4b5ec93 Compare September 9, 2017 15:06
@stack72
Copy link
Contributor Author

stack72 commented Sep 13, 2017

Hi @Ninir

I believe this is ready to go now, I removed the need for the Parsing of the Double and made the schema use schema.TypeDouble instead

Paul

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 14, 2017
@Ninir
Copy link
Contributor

Ninir commented Sep 15, 2017

Hi Paul,

Just checked and it looks very good. However, when updating the default value from the AWS console, the terraform plan does not see the change.

Would you have time to investigate?

Thanks! 👍

@Ninir Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 18, 2017
@stack72
Copy link
Contributor Author

stack72 commented Sep 23, 2017

@Ninir I don't have access to an AWS console to test this I am afraid

Can we follow up with this change in a different PR?

@Ninir
Copy link
Contributor

Ninir commented Sep 26, 2017

Hey @stack72

This will lead to expected issues from users, so I would prefer to get the case fixed at the time! 😅
If we are sure to get it in the same (next) release, then sure!

@endemics
Copy link

Hi @stack72 @Ninir is there any way for me to help get this accepted/merged?

@Ninir Ninir mentioned this pull request Oct 27, 2017
Merged
@Ninir
Copy link
Contributor

Ninir commented Oct 27, 2017

Hi @endemics Just started the review! i'm fixing the update from console issue in a first time :)

@endemics
Copy link

Thanks @Ninir, much appreciated!

stack72 and others added 2 commits October 30, 2017 18:28
@stack72 @Ninir
resource/aws_cloudwatch_log_metric_filter: Add support for DefaultValue
e733a99 
Fixes: hashicorp#1563

```
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudWatchLogMetricFilter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudWatchLogMetricFilter_ -timeout 120m
=== RUN   TestAccAWSCloudWatchLogMetricFilter_basic
--- PASS: TestAccAWSCloudWatchLogMetricFilter_basic (59.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	59.047s
```
@Ninir
flattenCloudWachLogMetricTransformations: Fixed default_value set bei…
f955381 
…ng a string rather than a float
@Ninir Ninir force-pushed the b-aws-logs-filter-1563 branch from 4b5ec93 to f955381 Compare October 30, 2017 17:45
@Ninir Ninir removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 30, 2017
Ninir
Ninir approved these changes Oct 30, 2017
View reviewed changes
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Hey @stack72

This is all ok now! :)
I allowed myself to fix conflicts & the read issue, and everything is all fine now:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudWatchLogMetricFilter_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudWatchLogMetricFilter_basic -timeout 120m
=== RUN   TestAccAWSCloudWatchLogMetricFilter_basic
--- PASS: TestAccAWSCloudWatchLogMetricFilter_basic (341.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	341.100s

Thanks for the work! 👍

@Ninir Ninir merged commit 2d165c7 into hashicorp:master Oct 30, 2017
@stack72 stack72 deleted the b-aws-logs-filter-1563 branch October 30, 2017 18:02
@iamam34 iamam34 mentioned this pull request Nov 20, 2017
Closed
@ghost
Copy link

ghost commented Apr 10, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ninir Ninir Ninir approved these changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Milestone
No milestone
4 participants
@stack72 @Ninir @endemics @radeksimko