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

feat: Add a homemade depends_on for MNG submodule to ensure ordering of resource creation #867

Merged

Conversation

barryib
Copy link
Member

@barryib barryib commented May 6, 2020

PR o'clock

Description

Hack to add a "homemade" depends_on as suggested https://discuss.hashicorp.com/t/tips-howto-implement-module-depends-on-emulation/2305/2 to ensure ordering of ressource creation for managed node groups.

I don't really like the approach, but the hack doesn't add complexity (because, there are only 2 resources in the submodule in which we add an explicit depends_on) and seems to work as expected.

I tested it to resolves #843

Note: I'm opening this PR to start discussion. So feedbacks are welcomed.

Questions

Checklist

@barryib barryib requested a review from dpiddockcmp May 6, 2020 12:21
@barryib barryib marked this pull request as draft May 6, 2020 12:24
@barryib barryib changed the title WIP: Add a homemade depends_on for MNG submodule to ensure ordering of resource creation feat: Add a homemade depends_on for MNG submodule to ensure ordering of resource creation May 6, 2020
@barryib
Copy link
Member Author

barryib commented May 6, 2020

closing in favor of #868

Copy link
Contributor

@dpiddockcmp dpiddockcmp left a comment

Choose a reason for hiding this comment

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

Please could we reconsider this?

It is a more terraform native way of ensuring dependency between resources.

I really regret the null_resource hack as I didn't know that you could just depend on variables! We're seeing this horror being repeated in the PRs for moving workers to a module and fargate support.

@@ -34,3 +34,11 @@ variable "node_groups" {
type = any
default = {}
}

# Hack for a homemade `depends_on` https://discuss.hashicorp.com/t/tips-howto-implement-module-depends-on-emulation/2305/2
# Will be removed when Terraform will add support of module's `depends_on`https://github.com/hashicorp/terraform/issues/10462
Copy link
Contributor

Choose a reason for hiding this comment

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

The module cannot depend on the aws_auth. Output from the module is used to generate the configmap's roles key.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

Humm after reading some issues, I think your comment #852 (comment) still apply. Terraform won't go into dependency cycle because the aws_auth_roles output from MNG is built with locals.

@barryib
Copy link
Member Author

barryib commented Jun 24, 2020

Please could we reconsider this?

It is a more terraform native way of ensuring dependency between resources.

I really regret the null_resource hack as I didn't know that you could just depend on variables! We're seeing this horror being repeated in the PRs for moving workers to a module and fargate support.

Yes of course. I will rebase my branch.

@barryib barryib force-pushed the tba/node-groups-depends-on branch from 8c3409b to 74efdb2 Compare June 24, 2020 07:40
@barryib barryib marked this pull request as ready for review June 24, 2020 07:40
@barryib barryib requested a review from dpiddockcmp June 24, 2020 07:47
@barryib barryib force-pushed the tba/node-groups-depends-on branch from 74efdb2 to dc959f2 Compare June 24, 2020 07:49
Copy link
Contributor

@dpiddockcmp dpiddockcmp 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

@barryib barryib merged commit 616d30e into terraform-aws-modules:master Jun 28, 2020
@barryib barryib deleted the tba/node-groups-depends-on branch June 28, 2020 00:31
barryib added a commit to Polyconseil/terraform-aws-eks that referenced this pull request Oct 25, 2020
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing tags causes node groups to be replaced.
2 participants