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

r/mwaa_environment - new resource #16616

Merged
merged 26 commits into from
Apr 5, 2021
Merged

r/mwaa_environment - new resource #16616

merged 26 commits into from
Apr 5, 2021

Conversation

shuheiktgw
Copy link
Collaborator

@shuheiktgw shuheiktgw commented Dec 6, 2020 •
edited
Loading

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 #16432

Release note for CHANGELOG:

* **New Resource:** `aws_mwaa_environment`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSMwaaEnvironment_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSMwaaEnvironment_basic -timeout 120m
=== RUN   TestAccAWSMwaaEnvironment_basic
=== PAUSE TestAccAWSMwaaEnvironment_basic
=== CONT  TestAccAWSMwaaEnvironment_basic
--- PASS: TestAccAWSMwaaEnvironment_basic (1934.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1936.124s
 
$ make testacc TESTARGS='-run=TestAccAWSMwaaEnvironment_disappears'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSMwaaEnvironment_disappears -timeout 120m
=== RUN   TestAccAWSMwaaEnvironment_disappears
=== PAUSE TestAccAWSMwaaEnvironment_disappears
=== CONT  TestAccAWSMwaaEnvironment_disappears
--- PASS: TestAccAWSMwaaEnvironment_disappears (1956.11s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1958.077s

$ make testacc TESTARGS='-run=TestAccAWSMwaaEnvironment_AirflowConfigurationOptions'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSMwaaEnvironment_AirflowConfigurationOptions -timeout 120m
=== RUN   TestAccAWSMwaaEnvironment_AirflowConfigurationOptions
=== PAUSE TestAccAWSMwaaEnvironment_AirflowConfigurationOptions
=== CONT  TestAccAWSMwaaEnvironment_AirflowConfigurationOptions
--- PASS: TestAccAWSMwaaEnvironment_AirflowConfigurationOptions (3321.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3323.728s

$ make testacc TESTARGS='-run=TestAccAWSMwaaEnvironment_LogConfiguration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSMwaaEnvironment_LogConfiguration -timeout 120m
=== RUN   TestAccAWSMwaaEnvironment_LogConfiguration
=== PAUSE TestAccAWSMwaaEnvironment_LogConfiguration
=== CONT  TestAccAWSMwaaEnvironment_LogConfiguration
--- PASS: TestAccAWSMwaaEnvironment_LogConfiguration (2580.88s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2582.884s

$ make testacc TESTARGS='-run=TestAccAWSMwaaEnvironment_full'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSMwaaEnvironment_full -timeout 120m
=== RUN   TestAccAWSMwaaEnvironment_full
=== PAUSE TestAccAWSMwaaEnvironment_full
=== CONT  TestAccAWSMwaaEnvironment_full
--- PASS: TestAccAWSMwaaEnvironment_full (1785.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1787.647s

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/mwaa Issues and PRs that pertain to the mwaa service. labels Dec 6, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Dec 6, 2020
@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XL Managed by automation to categorize the size of a PR. labels Dec 8, 2020
@CapChrisCap
Copy link

CapChrisCap commented Dec 9, 2020 •
edited
Loading

Hello @shuheiktgw ,
am also currently working on the MWAA resource: https://github.com/CapChrisCap/terraform-provider-aws/pull/1. Feel free to pick everything out of this pull request that you can maybe re-use for this pull request :-)

What is already done:

  • Resource implementation
  • Documentation
  • First successful acceptance tests (only update is missing)

Just message me, when you have questions about the structure and code. This is my first terraform provider extension of mine, so feedback is appreciated :-)

@shuheiktgw
Copy link
Collaborator Author

Thank you for your help, @CapChrisCap! 👍

This will speed up the process a lot. I'll definitely pick up the parts I haven't completed from your PR. I'll squash the commits after I complete the initial implementation, so I'll name you as a co-author of those squashed commits if you are fine with it!

@shuheiktgw
Copy link
Collaborator Author

I've been struggling with the creation_failure with an ACC test... I'm still actively debugging it so give me a little bit more time 🙇

@CapChrisCap
Copy link

@shuheiktgw I can understand your pain :-D I had the same issue. This is normally because the Subnets do not have any internet connection or are not private subnets: https://docs.aws.amazon.com/mwaa/latest/userguide/vpc-create.html and https://docs.aws.amazon.com/mwaa/latest/userguide/troubleshooting.html#env-create-fail. This was the reason, I created it with the CloudFormation template provided by AWS. The best is probably to use this first and then rewrite this into terraform. Notify me, when I can support ;-)

@shuheiktgw
Copy link
Collaborator Author

shuheiktgw commented Dec 16, 2020 •
edited
Loading

The last challenge I have is when I run make testacc TESTARGS='-run=TestAccAWSMwaaEnvironment_LogConfiguration' I encounter the following error. It is pretty close to the end I feel...

TestAccAWSMwaaEnvironment_LogConfiguration: resource_aws_mwaa_environment_test.go:193: Step 3/3 error: Error running apply:
Error: Provider produced inconsistent result after apply

When applying changes to aws_mwaa_environment.test, provider
"registry.terraform.io/hashicorp/aws" produced an unexpected new value: Root
resource was present, but now absent.

This is a bug in the provider, which should be reported in the provider's own
issue tracker.

@ghost ghost added the documentation Introduces or discusses updates to documentation. label Dec 18, 2020
shuheiktgw and others added 3 commits December 18, 2020 15:06
@shuheiktgw @CapCaWork
Add mwaa_environment
3732bdc 
Co-authored-by: Christoph Caprano <Christoph.Caprano@valiton.com>
@shuheiktgw @CapCaWork
Add tests for mwaa_environment
19ab800 
Co-authored-by: Christoph Caprano <Christoph.Caprano@valiton.com>
@shuheiktgw @CapCaWork
Add a documentation for mwaa_environment
60ca28b 
Co-authored-by: Christoph Caprano <Christoph.Caprano@valiton.com>
@shuheiktgw shuheiktgw force-pushed the add_resource_aws_mwaa_environment branch from 47d851f to 60ca28b Compare December 18, 2020 06:06
@shuheiktgw @CapCaWork
Use aws.StringValue instead
0217432 
Co-authored-by: Christoph Caprano <Christoph.Caprano@valiton.com>
@shuheiktgw
Copy link
Collaborator Author

@CapChrisCap Thank you so much for your support! It's all set so I'll turn this draft into a PR! 🎉 🎉

@shuheiktgw shuheiktgw marked this pull request as ready for review December 18, 2020 06:19
@shuheiktgw shuheiktgw requested a review from a team as a code owner December 18, 2020 06:19
@shuheiktgw shuheiktgw mentioned this pull request Dec 18, 2020
Closed
@bhushan-chirmade-tm
Copy link

Can somebody please merge this pull request? We are waiting for this feature to be available.

bhushan-chirmade-tm
bhushan-chirmade-tm approved these changes Jan 6, 2021
View reviewed changes
Copy link

@bhushan-chirmade-tm bhushan-chirmade-tm 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.

@lena-sadowska
Copy link

Thank you for the great work! We would also appreciate the merge as we are also waiting for this service

@john-jac
Copy link

john-jac commented Jan 7, 2021

Hi Folks...I'm the PM from the MWAA team. Please let me know if you need any help.

@bflad bflad added new-resource Introduces a new resource. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 31, 2021
@bflad bflad self-assigned this Mar 31, 2021
bflad
bflad suggested changes Mar 31, 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.

Hi @shuheiktgw 👋 Thank you so much for your work on this so far. Please see the below initial feedback/questions and let us know if you need help with anything.

Please also note that the acceptance testing in failing for me with errors like the following:

=== CONT  TestAccAWSMwaaEnvironment_basic
    resource_aws_mwaa_environment_test.go:57: Step 1/2 error: Error running apply: exit status 1

        Error: error creating MWAA Environment: ValidationException: Unable to check PublicAccessBlock configuration for the bucket tf-acc-test-4357126281653006425: The public access block configuration was not found (Service: S3, Status Code: 404, Request ID: 3AZ2R85VD2QNN0AV, Extended Request ID: bN/6jbXNsTVlJhteK3VjuUHjUaSbNdEEWmnRAwmNd45Y2nEPYYe8C17+VxjxWWD0kgJ9GBu3ebo=)

It appears that Step 5 of https://docs.aws.amazon.com/mwaa/latest/userguide/mwaa-s3-bucket.html is now being enforced during MWAA Environment creation, which likely means that we need to also include a aws_s3_bucket_public_access_block resource in the base test configuration such as:

resource "aws_s3_bucket_public_access_block" "test" {
  bucket = aws_s3_bucket.test.bucket

  block_public_acls   = true
  block_public_policy = true
}

aws/internal/service/mwaa/waiter/waiter.go Outdated
Comment on lines 13 to 14
// Amount of delay to check an environment status
EnvironmentCreatedDelay = 1 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

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

Open question: Is it possible to forego this mandatory 1 minute delay on creation? The waiter will automatically handle a nil response from the refresh function as a retryable condition. 👍

aws/internal/service/mwaa/waiter/waiter.go Outdated
Comment on lines 18 to 19
// Amount of delay to check an environment status
EnvironmentUpdatedDelay = 1 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

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

Open question: Can we lower this mandatory 1 minute delay to 15 seconds (or remove it altogether)?

aws/internal/service/mwaa/waiter/waiter.go Outdated
Comment on lines 23 to 24
// Amount of delay to check an environment status
EnvironmentDeletedDelay = 1 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here. 👍

website/docs/r/mwaa_environment.html.markdown Outdated

### Basic Usage

```hcl
Copy link
Contributor

Choose a reason for hiding this comment

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

Newer documentation linting will flag this:

Suggested change
```hcl
```terraform

website/docs/r/mwaa_environment.html.markdown Outdated

### Example with Airflow configuration options

```hcl
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```hcl
```terraform

aws/resource_aws_mwaa_environment_test.go Outdated
Comment on lines 756 to 757

depends_on = [aws_s3_bucket.test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
depends_on = [aws_s3_bucket.test]

aws/resource_aws_mwaa_environment_test.go Outdated
Comment on lines 765 to 766

depends_on = [aws_s3_bucket.test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
depends_on = [aws_s3_bucket.test]

aws/resource_aws_mwaa_environment_test.go Show resolved Hide resolved
aws/resource_aws_mwaa_environment_test.go Outdated
Comment on lines 534 to 535
"airflow.amazonaws.com",
"airflow-env.amazonaws.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"airflow.amazonaws.com",
"airflow-env.amazonaws.com"
"airflow.${data.aws_partition.current.dns_suffix}",
"airflow-env.${data.aws_partition.current.dns_suffix}"

aws/resource_aws_mwaa_environment_test.go Outdated
{
"Effect": "Allow",
"Principal": {
"Service": "logs.${data.aws_region.current.name}.amazonaws.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Service": "logs.${data.aws_region.current.name}.amazonaws.com"
"Service": "logs.${data.aws_region.current.name}.${data.aws_partition.current.dns_suffix}"

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 31, 2021
@bflad bflad linked an issue Mar 31, 2021 that may be closed by this pull request
Support Amazon Managed Workflows for Apache Airflow #16432
Closed
@shuheiktgw shuheiktgw force-pushed the add_resource_aws_mwaa_environment branch from 4ccd079 to 43ab58a Compare April 3, 2021 03:01
@shuheiktgw
Copy link
Collaborator Author

@bflad Thank you for your review! I confirmed all the ACC tests pass now so I believe I fixed it up. Would you review the PR again?

@shuheiktgw shuheiktgw requested a review from bflad April 5, 2021 05:30
bflad
bflad approved these changes Apr 5, 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.

Looks great, thank you, @shuheiktgw 🚀

Output from acceptance testing:

--- PASS: TestAccAWSMwaaEnvironment_full (1714.15s)
--- PASS: TestAccAWSMwaaEnvironment_basic (1957.20s)
--- PASS: TestAccAWSMwaaEnvironment_disappears (2239.96s)
--- PASS: TestAccAWSMwaaEnvironment_LogConfiguration (2531.17s)
--- PASS: TestAccAWSMwaaEnvironment_AirflowConfigurationOptions (3225.28s)

@bflad bflad merged commit e3ac6a9 into hashicorp:main Apr 5, 2021
@github-actions github-actions bot added this to the v3.36.0 milestone Apr 5, 2021
bflad added a commit that referenced this pull request Apr 5, 2021
@bflad
Update CHANGELOG for #16616
861d62c
@shuheiktgw
Copy link
Collaborator Author

Thank you all for your help! @CapChrisCap @ewbankkit @bflad

@shuheiktgw shuheiktgw deleted the add_resource_aws_mwaa_environment branch April 5, 2021 22:46
@ghost
Copy link

ghost commented Apr 9, 2021

This has been released in version 3.36.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 May 5, 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 May 5, 2021
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@john-jac john-jac john-jac left review comments

@CapChrisCap CapChrisCap CapChrisCap left review comments

@ewbankkit ewbankkit ewbankkit left review comments

@bhushan-chirmade-tm bhushan-chirmade-tm bhushan-chirmade-tm approved these changes

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/mwaa Issues and PRs that pertain to the mwaa service. size/XXL 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.36.0
Development

Successfully merging this pull request may close these issues.

Support Amazon Managed Workflows for Apache Airflow