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/outputs from multi paths #407

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Fix/outputs from multi paths #407

merged 2 commits into from
Feb 7, 2025

Conversation

jgresty
Copy link
Member

@jgresty jgresty commented Feb 7, 2025

The vervet config allows both Path and Paths to be defined on the output.
GetOutputSpecs assumed that only the former was populated so wouldn't return specs if
only the latter was populated.

Having both Output.Path and Output.Paths was a source of errors waiting to
happen if the caller didn't call ResolvePaths first. This patch resolves the
paths when deserialising so it reduces the mental load overall.

The vervet config allows both Path and Paths to be defined on the output. This
function assumed that only the former was populated so wouldn't return specs if
only the latter was populated.

This seems like a footgun that will happen again so it would be good to change
the types so this doesn't happen again.
@jgresty jgresty requested a review from a team as a code owner February 7, 2025 09:36
@jgresty jgresty force-pushed the fix/outputs-from-multi-paths branch 3 times, most recently from 2fa81be to b66104f Compare February 7, 2025 10:20
Having both Output.Path and Output.Paths was a source of errors waiting to
happen if the caller didn't call ResolvePaths first. This patch resolves the
paths when deserialising so it reduces the mental load overall.
@jgresty jgresty force-pushed the fix/outputs-from-multi-paths branch from b66104f to e7c2f64 Compare February 7, 2025 10:25
@jgresty jgresty merged commit 6313c82 into main Feb 7, 2025
13 checks passed
@jgresty jgresty deleted the fix/outputs-from-multi-paths branch February 7, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants