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

Fixes overriding modules which have depends_on attribute set #32796

Merged
merged 1 commit into from Mar 9, 2023

Conversation

ekini
Copy link
Contributor

@ekini ekini commented Mar 7, 2023

Fixes #32795

Target Release

1.4.x

Draft CHANGELOG entry

BUG FIXES

  • Enables overriding modules that have depends_on attribute set (although still doesn't allow to override depends_on , which is the intended behaviour)

@crw
Copy link
Collaborator

crw commented Mar 7, 2023

Thanks for this submission!

@crw crw added the bug label Mar 7, 2023
@liamcervante liamcervante added the 1.4-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Mar 8, 2023
@liamcervante liamcervante self-requested a review March 8, 2023 12:27
@liamcervante
Copy link
Member

liamcervante commented Mar 8, 2023

This fix looks good to me, before we merge could you extend one of the tests to cover using depends_on in this context?

There's some test data in testdata/valid-modules/override-module you can use. If you just add a depends on statement to the module in primary.tf: depends_on = ["example_resource.resource"] that will make the a test in module_merge_test.go fail. You can then make that test pass by updating the expected data in the test to include the the new depends_on data and means we'll capture this fix in any changes going forward.

Thanks!

@liamcervante liamcervante added the waiting-response An issue/pull request is waiting for a response from the community label Mar 8, 2023
@ekini ekini force-pushed the fix/modules_merge_depends branch from da08593 to 2409be4 Compare March 9, 2023 10:20
@ekini
Copy link
Contributor Author

ekini commented Mar 9, 2023

Thank you for the feedback @liamcervante ! Added the test and it passes :)

@liamcervante liamcervante removed the waiting-response An issue/pull request is waiting for a response from the community label Mar 9, 2023
@liamcervante liamcervante merged commit 36aac6b into hashicorp:main Mar 9, 2023
@github-actions
Copy link

github-actions bot commented Mar 9, 2023

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

github-actions bot commented Apr 9, 2023

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 contributions.
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 Apr 9, 2023
@ekini ekini deleted the fix/modules_merge_depends branch September 19, 2023 07:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.4-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fail with "The depends_on argument may not be overridden." for module that doesn't get overridden
3 participants