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

fix(terraform): pass module outputs to other modules #6300

Closed
wants to merge 3 commits into from

Conversation

nikpivkin
Copy link
Contributor

Description

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@fwereade
Copy link
Contributor

FWIW: I think I've had good results from re-evaluating submodules when their inputs change. You can see the approach here (against an old defsec which I currently depend on) – I think it currently evaluates modules-with-outputs one more time than it needs to, but the performance cost doesn't seem excessive and the strategy seems to align well with how the rest of the package works.

The specific test case I've been working against is here; backporting the approach in this PR doesn't appear to address that scenario, but that may be due to other changes since v0.93.1.

(I don't claim any particular expertise here, but I hope this is relevant/helpful. And I'd like to update the tfparse dependency to point at current code, but I can't commit to a timeframe for that.)

@nikpivkin
Copy link
Contributor Author

nikpivkin commented Mar 20, 2024

Hi @fwereade !

Thanks for the research! I will familiarise myself with your solution and give feedback.

@nikpivkin
Copy link
Contributor Author

nikpivkin commented Mar 21, 2024

@fwereade Looks great! Do you mind if I open a PR with your changes or maybe you want to do it yourself?

@fwereade
Copy link
Contributor

@nikpivkin thanks! please go ahead and open a PR, I can't promise when I'd get to it myself

FWIW, I think it could use a little improvement: I'm pretty lastInputs/lastOutputs should be replaced with a lastState from ctx.Get("module", name) after setting the outputs (and the DeepEqual check against the same), so that a module with outputs doesn't misinterpret them as new inputs and re-eval; this would also render the extracted GetInputVars redundant

…but I'll leave it to your judgment :)

@nikpivkin
Copy link
Contributor Author

Closed in favor of #6411

@nikpivkin nikpivkin closed this Mar 28, 2024
@nikpivkin nikpivkin deleted the tf-modules-ctx branch March 28, 2024 11:47
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.

bug(terraform): module output values are not passed into the context of other modules
2 participants