manifest: change computed_field logic to mark all as unknown instead of only on changes
#2432
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
latest commit solves the issue from referenced issue, however we still run into this error if the user chooses to set computed_fields and not include If the solution to this looks good then i think the only thing left is to add a test for this case to check that annotations/labels are seen as |
BBBmau
changed the title
|
I've attempted to run the test on my machine but running into issues, could be because im not using kind and instead using minikube: |
BBBmau
commented
Mar 2, 2024
BBBmau
changed the title
computed_field logic to mark all as unknown instead of only on changes
alexsomesan
requested changes
Mar 28, 2024
There was a problem hiding this comment.
We still need logic to look up the original value of the computed attribute in "manifest" with the appropriate attribute path adaptations, to avoid the "attribute disappeared" errors.
We also need to revert back to only setting values to computed during planning if they actually changed in configuration. Otherwise, values modified by the API during apply (such as "cpu" in the test example) will trigger a perpetual diff.
Here's an example function for looking up the values in manifest, that I tested and found to work decently:
// findBackfillValue tries to optimistically find an attribute value pointed to by an attribute path (ap) inside
// a complex type container value (m) by making semantically equivalent adaptations to the attribute path steps.
// It considers Object and Map types as semantically equivalent AFA indexing attributes goes.
func findBackfillValue(m interface{}, ap *tftypes.AttributePath) (interface{}, *tftypes.AttributePath, error) {
v, restPath, err := tftypes.WalkAttributePath(m, ap)
if err != nil {
if len(restPath.Steps()) > 0 {
// Attribute might not be found, because the attribute path step type doesn't matcht
// the container type being indexed (core parses HCL to only Object and Tupple, but not Map and List).
// In that case, the attribute paths constructed for values in "object"
// will need adjusting for type differences between "manifest"
fs := restPath.Steps()[0]
switch e := fs.(type) {
case tftypes.ElementKeyString:
// if expecting a Map, try indexing with AttributeName instead
tp := tftypes.NewAttributePath().WithAttributeName(string(e))
tv, rp, err := tftypes.WalkAttributePath(v, tp)
if err != nil {
return v, rp, err
}
return findBackfillValue(tv, tftypes.NewAttributePathWithSteps(restPath.Steps()[1:]))
case tftypes.AttributeName:
// if expecting an Object, try indexing with ElementKeyString instead
tp := tftypes.NewAttributePath().WithElementKeyString(string(e))
tv, rp, err := tftypes.WalkAttributePath(v, tp)
if err != nil {
return v, rp, err
}
return findBackfillValue(tv, tftypes.NewAttributePathWithSteps(restPath.Steps()[1:]))
}
}
return nil, nil, err
}
return v, restPath, err
}
manifest/test/acceptance/testdata/ComputedFields/computed_deployment.tf
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Somesan <alex.somesan@gmail.com>
…oyment.tf Co-authored-by: Alex Somesan <alex.somesan@gmail.com>
|
Latest commit addresses the case where API changes the config value set by the user. How it appears in config: How it appears in YAML |
|
resource "kubernetes_manifest" "daemonset_prometheus_daemonset" {
manifest = {
apiVersion = "apps/v1"
kind = "DaemonSet"
metadata = {
name = "prometheus-daemonset"
namespace = "default"
}
spec = {
selector = {
matchLabels = {
name = "prometheus-exporter"
tier = "monitoring"
}
}
template = {
metadata = {
labels = {
name = "prometheus-exporter"
tier = "monitoring"
}
}
spec = {
containers = [
{
image = "prom/node-exporter"
name = "prometheus"
ports = [
{
containerPort = 80
},
]
},
]
}
}
}
}
} # kubernetes_manifest.daemonset_prometheus_daemonset will be updated in-place
~ resource "kubernetes_manifest" "daemonset_prometheus_daemonset" {
~ object = {
~ metadata = {
~ annotations = {
- "deprecated.daemonset.template.generation" = "1"
} -> (known after apply)
+ labels = (known after apply)
name = "prometheus-daemonset"
# (12 unchanged attributes hidden)
}
# (3 unchanged attributes hidden)
}
# (1 unchanged attribute hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.Changes to the |
|
Hi ! Is there anything blocking this PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The idea is to treat all values in
computed_fieldas anUnknownValue, before the values would only be treated as anUnknownValuewhen a change was present. This will insure that values in annotations/labels are covered especially in cases where the user did not set the label/annotation themselves.Fixes #1591
Acceptance tests
Output from acceptance testing:
Release Note
Release note for CHANGELOG:
References
Community Note