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: update yaml validation to match kong behavior #37

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

omegabytes
Copy link
Contributor

@omegabytes omegabytes commented Dec 14, 2022

Downgrades yaml package so that goks has behavioral parity with kong yaml validation.

@omegabytes omegabytes requested a review from a team as a code owner December 14, 2022 23:41
@CLAassistant
Copy link

CLAassistant commented Dec 14, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@GGabriele GGabriele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if there is any upstream GitHub Issue on the yaml project about why this doesn't work with v3 but it does with v1?

@mikefero
Copy link
Contributor

@omegabytes Please clean up the description in this PR to reflect this change.

@aboudreault
Copy link
Contributor

Should we standardize what we use for yaml across our different projects (if possible)? e.g. Kong/deck#800

@omegabytes
Copy link
Contributor Author

Do you know if there is any upstream GitHub Issue on the yaml project about why this doesn't work with v3 but it does with v1?

I scanned the open + closed issues for go-yaml but didn't find a directly related issue.

@omegabytes
Copy link
Contributor Author

Should we standardize what we use for yaml across our different projects (if possible)? e.g. Kong/deck#800

Strongly agree with standardizing, at least for yaml packages. Luckily downgrading to v1 was a simple fix (thanks @mikefero).

I've also considered standing up complete unit testing that asserts that yaml handling has behavioral parity with kong's yaml handling. That's a big task so I'd like to avoid it unless absolutely necessary. We'd probably need pretty exhaustive unit tests in both kong and in goks (and in deck, and so on).

@omegabytes omegabytes merged commit 9ae340e into main Dec 20, 2022
@omegabytes omegabytes deleted the fix/yaml-validation branch December 20, 2022 19:28
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.

5 participants