-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Supports dynamic blocks in tags and labels #31
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
Conversation
* main: chore: Updates Go to 1.24.1 and linter (#32)
internal/convert/convert.go
Outdated
return nil, fmt.Errorf("dynamic block %s: %s or %s not found", name, nKey, nValue) | ||
} | ||
collectionExpr := strings.TrimSpace(string(d.forEach.Expr().BuildTokens(nil).Bytes())) | ||
keyExpr := strings.TrimSpace(strings.ReplaceAll(string(key.Expr().BuildTokens(nil).Bytes()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot happening on this line. Maybe create an intermediate variable?
Or
Extract this into a function. for example, removeDynamicVarPrefx()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, changed here: 24b5389
} | ||
return nil | ||
} | ||
|
||
type dynamicBlock struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice abstraction!
priority = 7 | ||
} | ||
} | ||
labels { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Quite a big example! Static values and dynamic!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Curious how much logic can be used for other attributes in the sense of mapping generic dynamic
/for_each
/content
over to respective for
/in
/ value of object
tags { | ||
key = var.tag_key # non-literal values are supported and enclosed in parentheses | ||
value = var.tag_value | ||
} | ||
dynamic "tags" { | ||
for_each = var.tags | ||
content { | ||
key = tags.key | ||
value = replace(tags.value, "/", "_") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice that we support the possibility of dynamic + literal blocks at the same time
] | ||
} | ||
] | ||
tags = var.tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we assuming that var.tags
is map here? Is there a case where it could be a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all examples I've found are maps, if you find any example of a list, please let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I believe it will good to keep track of these assumptions mainly for our future reference and transparency for users. Maybe a small note within Dynamic blocks in tags and labels
section in the readme file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @AgustinBettati , added in this follow-up PR: #33
@AgustinBettati I think we'll be able to do some reuse, for example the dynamicBlock struct and surely some funcs |
Description
Supports dynamic blocks
Link to any related issue(s): CLOUDP-303941
Type of change:
Required Checklist:
Further comments