community.general
community.general copied to clipboard
ansible-collections
terraform: Return changed: true when plan file is generated with diff
SUMMARY
When using the module with state: planned, the module would never return changed: true, even if its own needs_application function returned true.
This behavior seems strange, and the associated documentation note to run plans in check mode, even though state: planned is an option, seems confusing.
In #807, the same behavior is mentioned, though their issue is also lack of output in check mode, which might require additional work.
My proposal is that when needs_application: true, meaning that Terraform has generated a new plan file,
and there is a diff against existing infrastructure; the module should return changed: true.
This, in my mind, mirrors the behavior of terraform itself, which returns a non-zero code when a plan file differs from running infrastructure, and requires application to eliminate the drift.
I am categorizing this is a a feature request, and invite others to discuss this proposal.
ISSUE TYPE
- Feature Pull Request
COMPONENT NAME
plugins/modules/cloud/misc/terraform.py
ADDITIONAL INFORMATION
Integration tests are included with assertions to test the new behavior.
I have added a changelog fragment. I classified it under minor_changes based on the guidance in the docs.
The test ansible-test sanity --test changelog [explain] failed with 1 error:
changelogs/fragments/4673-terraform-change-state.yml:0:0: section "minor_changes" list items must be type str not dict
The test ansible-test sanity --test changelog [explain] failed with 1 error:
changelogs/fragments/4673-terraform-change-state.yml:0:0: section "minor_changes" list items must be type str not dict
The test ansible-test sanity --test changelog [explain] failed with 1 error:
changelogs/fragments/4673-terraform-change-state.yml:0:0: section "minor_changes" list items must be type str not dict
The test ansible-test sanity --test changelog [explain] failed with 1 error:
changelogs/fragments/4673-terraform-change-state.yml:0:0: section "minor_changes" list items must be type str not dict
Hmm, this seems to be a breaking behavior change to me. While check_mode: true should return the same results than check_mode: false if possible, this is something else. I'm not sure whether this makes sense, but it has been the behavior of this module for a long time, so changing it requires a deprecation period and advance warning to users.
I agree with you that it's a breaking change for certain possible use cases. The Ansible docs mentioned not using breaking change for individual components of a collection. If you're OK with the breaking change/major change designation, I will update it.
I agree with you that it's a breaking change for certain possible use cases. The Ansible docs mentioned not using breaking change for individual components of a collection. If you're OK with the breaking change/major change designation, I will update it.
We do not accept breaking changes without a deprecation period first. Quoting from #582
Deprecations are expected to have a deprecation cycle of at least 2 major versions (i.e. ~1 year). Maintainers can use a longer deprecation cycle if they want to support the old code for that long.
So basically you have to emit a deprecation warning in this situation that the behavior will change. Since such deprecations are really annoying to users, it's better to proceed as follows:
- Add a new option which allows to switch between the current behavior and the new behavior. Default value is for the current behvaior.
- In a later minor release, deprecate the current default vaule of the new option. The deprecation message should say that the default value will change in community.general 7.0.0 (or something like that), and that users can get rid of the warning by explicitly specifying a value for this option.
- In community.general 7.0.0 (or whatever was picked above) the default value will change for the option and the new behavior will be the default.
@geekifier This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.
Please note that in #5461 the collection repository was restructured to remove the directory tree in plugins/modules/, and the corresponding tree in tests/unit/plugins/modules/. Your PR modifies files in this directory structure, and unfortunately now has some conflicts, potentially because of this. Please rebase with the current main branch to remove the conflicts.
@geekifier You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.