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

Dpf 220/ecs department roles #1979

Merged
merged 16 commits into from
Nov 14, 2024
Merged

Conversation

timburke-hackit
Copy link
Collaborator

@timburke-hackit timburke-hackit commented Nov 13, 2024

Creates base department roles for running ECS tasks that are broadly equivalent to the existing Glue roles.

@timburke-hackit
Copy link
Collaborator Author

timburke-hackit commented Nov 14, 2024

Sorry about the mess of commits on this one. Was going to tidy up but will just squash and merge to keep main reasonably sensible as it's actually not got that much going on.

@timburke-hackit timburke-hackit marked this pull request as ready for review November 14, 2024 08:34
Copy link
Collaborator

@Tian-2017 Tian-2017 left a comment

Choose a reason for hiding this comment

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

Nice work! At first glance, I was wondering how it dynamically creates the role for each department without using for_each. Then I realised there’s a main department.tf file in the core folder, which makes sense.

Looks the ecs_department_policy miss the glue access, but feel free to merge, we can refine it later.

Copy link
Collaborator

@LBH-wgreeff LBH-wgreeff left a comment

Choose a reason for hiding this comment

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

LGTM

@timburke-hackit
Copy link
Collaborator Author

Looks the ecs_department_policy miss the glue access, but feel free to merge, we can refine it later.

I figured they probably weren't needed because if you need an onward Glue job or crawler you'd likely be triggering that from Airflow. But yeah, if it turns out we need them it's easy enough to add another policy to do that. 👍

@timburke-hackit timburke-hackit merged commit 435b4f1 into main Nov 14, 2024
12 checks passed
@timburke-hackit timburke-hackit deleted the dpf-220/ecs-department-roles branch November 14, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants