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

Allow zero value for cookie_expiration_period #17204

Merged
merged 5 commits into from
Feb 16, 2021
Merged

Allow zero value for cookie_expiration_period #17204

merged 5 commits into from
Feb 16, 2021

Conversation

bill-rich
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #12678

Release note for CHANGELOG:

Allow zero value for cookie_expiration_period in `aws_lb_cookie_stickiness_policy` resource.

Output from acceptance testing:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSLBCookieStickinessPolicy_basic -timeout 120m
=== RUN   TestAccAWSLBCookieStickinessPolicy_basic
=== PAUSE TestAccAWSLBCookieStickinessPolicy_basic
=== CONT  TestAccAWSLBCookieStickinessPolicy_basic
--- PASS: TestAccAWSLBCookieStickinessPolicy_basic (27.89s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	27.935s

@bill-rich bill-rich requested a review from a team as a code owner January 21, 2021 01:07
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/elb Issues and PRs that pertain to the elb service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jan 21, 2021
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 21, 2021
@bill-rich bill-rich removed the needs-triage Waiting for first response or review from a maintainer. label Jan 21, 2021
Base automatically changed from master to main January 23, 2021 01:00
@bflad bflad self-assigned this Feb 4, 2021
bflad
bflad approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Some nits about the testing, but otherwise looks good according to the API reference. 🚀 Can you please also add a .changelog/ entry?

Output from acceptance testing:

--- PASS: TestAccAWSLBCookieStickinessPolicy_disappears_ELB (22.25s)
--- PASS: TestAccAWSLBCookieStickinessPolicy_disappears (27.80s)
--- PASS: TestAccAWSLBCookieStickinessPolicy_basic (42.58s)

aws/resource_aws_lb_cookie_stickiness_policy_test.go
@@ -182,7 +183,7 @@ resource "aws_lb_cookie_stickiness_policy" "foo" {
name = "foo-policy"
load_balancer = aws_elb.lb.id
lb_port = 80
cookie_expiration_period = 300
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically it is preferable to create new test cases rather than adjust existing tests to ensure we don't miss any regressions, but in this case it is probably okay. Just note that the comment on line 167 is now incorrect and we should probably be verifying these state values with resource.TestCheckResourceAttr() in the test functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it was probably ok in this case since it was still testing the update functionality. I got the comment fixed and also added checks for the cookie_expiration_period.

@bflad bflad added the bug Addresses a defect in current functionality. label Feb 4, 2021
@bill-rich bill-rich merged commit 4a030f4 into main Feb 16, 2021
@bill-rich bill-rich deleted the b_allow_zero_cookie_expiration_period branch February 16, 2021 22:28
@github-actions github-actions bot added this to the v3.29.0 milestone Feb 16, 2021
github-actions bot pushed a commit that referenced this pull request Feb 16, 2021
Update CHANGELOG.md for #17204
20e33f1
heitorlessa added a commit to heitorlessa/terraform-provider-aws that referenced this pull request Feb 18, 2021
@heitorlessa
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
e1da594 
…r-aws into main

* 'main' of https://github.com/hashicorp/terraform-provider-aws: (599 commits)
  Adds CHANGELOG
  Updates acceptance tests and documentation
  Update CHANGELOG.md for hashicorp#17650
  Removes redundant error check
  Update CHANGELOG.md for hashicorp#17645
  resource/aws_lb_listener_certificate: Prevent resource ID parsing error with IAM Server Certificate names containing underscores (hashicorp#17645)
  resource/aws_instance: Apply attribute waiter logic to iam_instance_profile attribute (hashicorp#17414)
  Update CHANGELOG.md for hashicorp#17534
  Adds multi-error handling to ElastiCache Cluster sweeper
  Excludes Clusters in Replication Groups from sweeper
  Adds check for deleting only member of replication group and does not retry
  Update CHANGELOG.md for hashicorp#17204
  Update docs for cloudfront_distribution resource
  fix(aws_ses_reciept_rule): include period in name validation regex
  Add CHANGELOG entry.
  r/aws_dx_gateway_association: Fix 'TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount'.
  r/aws_ec2_transit_gateway: Increase deletion IncorrectState retry to 5 minutes.
  Update CHANGELOG.md after release
  fix if evaluation
  r/aws_dx_gateway_association: Modernize acceptance tests.
  ...
heitorlessa added a commit to heitorlessa/terraform-provider-aws that referenced this pull request Feb 18, 2021
@heitorlessa
Merge branch 'main' into feat/eventbridge_replay
f1a4348 
* main: (599 commits)
  Adds CHANGELOG
  Updates acceptance tests and documentation
  Update CHANGELOG.md for hashicorp#17650
  Removes redundant error check
  Update CHANGELOG.md for hashicorp#17645
  resource/aws_lb_listener_certificate: Prevent resource ID parsing error with IAM Server Certificate names containing underscores (hashicorp#17645)
  resource/aws_instance: Apply attribute waiter logic to iam_instance_profile attribute (hashicorp#17414)
  Update CHANGELOG.md for hashicorp#17534
  Adds multi-error handling to ElastiCache Cluster sweeper
  Excludes Clusters in Replication Groups from sweeper
  Adds check for deleting only member of replication group and does not retry
  Update CHANGELOG.md for hashicorp#17204
  Update docs for cloudfront_distribution resource
  fix(aws_ses_reciept_rule): include period in name validation regex
  Add CHANGELOG entry.
  r/aws_dx_gateway_association: Fix 'TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount'.
  r/aws_ec2_transit_gateway: Increase deletion IncorrectState retry to 5 minutes.
  Update CHANGELOG.md after release
  fix if evaluation
  r/aws_dx_gateway_association: Modernize acceptance tests.
  ...
@ghost
Copy link

ghost commented Feb 19, 2021

This has been released in version 3.29.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Mar 19, 2021

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 as resolved and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
bug Addresses a defect in current functionality. service/elb Issues and PRs that pertain to the elb service. size/XS Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v3.29.0
Development

Successfully merging this pull request may close these issues.

aws_lb_cookie_stickiness_policy doesn't support cookie_expiration_period = 0
2 participants
@bill-rich @bflad