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

mwaa v5 migration #248

Merged
merged 9 commits into from
Jul 24, 2023
Merged

mwaa v5 migration #248

merged 9 commits into from
Jul 24, 2023

Conversation

bbgu1
Copy link
Contributor

@bbgu1 bbgu1 commented Jul 14, 2023

What does this PR do?

🛑 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.

Update mwaa blueprint to v5

Motivation

EKS blueprint v5 changes

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • [] Mandatory for new blueprints. Yes, I have added a example to support my blueprint PR
  • Mandatory for new blueprints. Yes, I have updated the website/docs or website/blog section for this feature
  • Yes, I ran pre-commit run -a with this PR. Link for installing pre-commit locally

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 14, 2023 13:24 — with GitHub Actions Inactive
@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 14, 2023 14:56 — with GitHub Actions Inactive
@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 14, 2023 15:03 — with GitHub Actions Inactive
@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 14, 2023 15:04 — with GitHub Actions Inactive
vara-bonthu
vara-bonthu reviewed Jul 15, 2023
View reviewed changes
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

Thanks @bbgu1 for the MWAA upgrade PR.
I have left some comments to update.

schedulers/terraform/managed-airflow-mwaa/addons.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/addons.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/addons.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/cleanup.sh Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/eks.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/providers.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/addons.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/data.tf Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/addons.tf Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/cleanup.sh Outdated Show resolved Hide resolved
@vara-bonthu
Copy link
Contributor

@bbgu1 Could you please resolve conflicts and double check with our internal blueprint standards. I will review and merge it accordingly.

@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 21, 2023 18:04 — with GitHub Actions Inactive
@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 21, 2023 18:13 — with GitHub Actions Inactive
@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 21, 2023 18:17 — with GitHub Actions Inactive
@bbgu1
Copy link
Contributor Author

bbgu1 commented Jul 21, 2023

@vara-bonthu all resolved. Had quite a lot trouble this week due to the terraform-aws-modules/security-group/aws module failing to create rules for vpc_endpoints_sg during my tests. Took a while to find the issue and I am still working on finding the cause of it. For this blueprint I switched to use the terraform aws_security_group module which IMO is much cleaner.

vara-bonthu
vara-bonthu reviewed Jul 22, 2023
View reviewed changes
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

@bbgu1 LGTM 👍🏼 Two minor comments and thenwe are good to merge.

schedulers/terraform/managed-airflow-mwaa/install.sh Outdated Show resolved Hide resolved
schedulers/terraform/managed-airflow-mwaa/mwaa.tf Outdated Show resolved Hide resolved
@vara-bonthu
Copy link
Contributor

@vara-bonthu all resolved. Had quite a lot trouble this week due to the terraform-aws-modules/security-group/aws module failing to create rules for vpc_endpoints_sg during my tests. Took a while to find the issue and I am still working on finding the cause of it. For this blueprint I switched to use the terraform aws_security_group module which IMO is much cleaner.

Thank you for upgrading this blueprint and fixing the issue 👍🏼

@bbgu1 bbgu1 temporarily deployed to DoEKS Test July 24, 2023 04:13 — with GitHub Actions Inactive
@bbgu1
Copy link
Contributor Author

bbgu1 commented Jul 24, 2023

Fixed the two files you mentioned - thanks for reviewing.

vara-bonthu
vara-bonthu approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

LGTM👍

@vara-bonthu vara-bonthu merged commit 284a54e into awslabs:main Jul 24, 2023
44 of 47 checks passed
@vara-bonthu vara-bonthu mentioned this pull request Jul 25, 2023
Closed
5 tasks
vara-bonthu pushed a commit that referenced this pull request Jul 28, 2023
@bbgu1 @vara-bonthu
mwaa v5 migration (#248)
a541d79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vara-bonthu vara-bonthu vara-bonthu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bbgu1 @vara-bonthu