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

Refactor spec.go to remove UnmarshalYAML boilerplate #161

Merged
merged 8 commits into from
Aug 24, 2023

Conversation

drevell
Copy link
Contributor

@drevell drevell commented Aug 22, 2023

Before this, each UnmarshalYAML function was a copy-pasted block of 20-ish lines of boilerplate. With this change, each UnmarshalYAML function turns into a one-liner which calls unmarshalPlain.

This requires a somewhat regrettable amount of reflection code, but it's arguably no more confusing than what was there before, while having the benefit of being more concise and DRY.

Fixes #30.

I'm attempting to use stacked PRs here; I intend to switch the base branch of this PR to main once the drevell/for-each-values-from branch is merged.

Previously, we could only iterate over a hardcoded YAML list of values.
This PR adds the `values_from: 'some_cel_expression'` field to
programmatically compute a list of strings to iterate over.

We may want to add caching of compiled CEL programs in the future to
avoid paying a few milliseconds per compilation. We don't currently
have any use cases where a for_each action will run often enough for
that to matter, but it's conceivable that the latency would be
noticeable for deeply-nested for_each actions.

Still to do: write docs and examples
Before this, each UnmarshalYAML function was a copy-pasted block of
20-ish lines of boilerplate. With this change, each UnmarshalYAML
function turns into a one-liner which calls unmarshalPlain.

This requires a somewhat regrettable amount of reflection code, but it's
arguably no more confusing than what was there before, while having the
benefit of being more concise and DRY.

Fixes #30.
@drevell drevell requested a review from a team as a code owner August 22, 2023 21:59
@drevell drevell requested a review from sethvargo August 22, 2023 21:59
sethvargo
sethvargo previously approved these changes Aug 22, 2023
Base automatically changed from drevell/for-each-values-from to main August 23, 2023 18:01
@drevell drevell dismissed sethvargo’s stale review August 23, 2023 18:01

The base branch was changed.

@drevell drevell requested a review from a team as a code owner August 23, 2023 18:01
@drevell drevell requested a review from sethvargo August 23, 2023 18:59
templates/model/spec.go Show resolved Hide resolved
@drevell drevell merged commit 7038f46 into main Aug 24, 2023
2 checks passed
@drevell drevell deleted the drevell/unmarshalyaml branch August 24, 2023 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Clean up tech debt in YAML unmarshaling
3 participants