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: Skip computed stat fields #299

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

dokmic
Copy link
Contributor

@dokmic dokmic commented Nov 12, 2023

While working with the provider, I discovered that it changes the Terraform state on every apply, even though there are no changes. That's caused by constantly changing stats fields (e.g., number of transferred bytes or packets). That breaks the whole concept of the Terraform state and produces state changes without actual "infrastructure" change.

Apart from that, some resources returned many warnings related to the missing schema fields.

This PR tries to address those problems in a backward-compatible way by reusing the MetaSkipFields property:

  1. Skip stats for the ethernet resources, which were causing a lot of noise in the output (resolves Schema warnings when managing Ethernet Interfaces #298).
  2. Skip stats for the firewall rules.
  3. Skip debug fields from the bridge ports.
  4. Skip the last login information for the system users.
  5. Add the missing computed field to the system logging resource.

As this PR doesn't explicitly add any new schema fields, the changes are compatible with any supported RouterOS version.

@dokmic dokmic requested a review from a team as a code owner November 12, 2023 23:06
@dokmic dokmic force-pushed the bugfix/missing-properties branch 2 times, most recently from a279e53 to bb7e4c3 Compare November 13, 2023 07:38
@vaerh
Copy link
Collaborator

vaerh commented Nov 13, 2023

There's a big problem: someone can use some of these fields and we'll get a script that doesn't work.
If you have relevant experience, I wouldn't mind an answer to the question why changing the state file breaks the terraforming idea? I've looked at the code of other providers but I can't find how they handle changing fields.
Somewhere I'm thinking that the counters should probably be moved to the data sources, but that's unrealistic.

@dokmic
Copy link
Contributor Author

dokmic commented Nov 13, 2023

@vaerh Changing state breaks the main principle of IaC -- idempotence.

Idempotence, the ability of a given operation to always produce the same result, is a crucial IaC principle. A deployment command always sets the target environment into the same configuration, regardless of the environment's starting state.

In our case, the state was constantly changing, requiring reuploading the state to the selected backend. This is very inefficient and may lead to extra costs and make debugging more difficult.

I think there is no practical or theoretical use case when you need counters in our infrastructure. The resource fields are usually used as dependencies in other resources or outputs, and conditions based on counters will be unreliable. It would make sense to have them in the data sources, but again, that shouldn't be used in any conditions. And the whole idea of having a script (which is imperative) is against the declarative model of Terraform language:

The Terraform language is declarative, describing an intended goal rather than the steps to reach that goal.

On top of that, having more defined schema fields requires a little bit more maintenance as it's harder to predict them and, therefore, test them. So, pragmatically, excluding them from the resource definitions is better. In the worst case, people using those fields will come back here after bumping the provider.

@vaerh
Copy link
Collaborator

vaerh commented Nov 13, 2023

Thanks!
I thought that the feedback of the system in the form of a state file does not affect idempotence.
It is likely that we will have to remove all counters from resources.
I need some time to think about this.

@vaerh vaerh merged commit 579f0a0 into terraform-routeros:main Nov 15, 2023
3 checks passed
@vaerh
Copy link
Collaborator

vaerh commented Nov 15, 2023

🎉 This PR is included in version 1.25.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@vaerh vaerh added the released label Nov 15, 2023
@dokmic dokmic deleted the bugfix/missing-properties branch November 15, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema warnings when managing Ethernet Interfaces
2 participants