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

Replace use of yaml.v2 library with yaml.v3 #7091

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

anderseknert
Copy link
Member

Some quirks encountered in the upgrade:

  • Errors reported by the parser are now zero-indexed
  • A few errors seemingly reported on the line after the error rather than where it happened
  • Also a few tests where the line number reported previously seemed wrong but now is right
  • Something different in how the parser unmarshalled to the "raw" schema annotation type we used. Changed to use a a map instead of that type alias.. but I'm not really sure why that had to be done

Perhaps worth looking into this further. But pushing this now to start that discussion.

Fixes #7090

@anderseknert anderseknert changed the title Signed-off-by: Anders Eknert <[email protected]> Replace use of yaml.v2 library with yaml.v3 Oct 2, 2024
@@ -2397,7 +2394,7 @@ func (b *metadataParser) Parse() (*Annotations, error) {
if err != nil {
return nil, err
}
case map[interface{}]interface{}:
case map[string]any:
Copy link
Member

Choose a reason for hiding this comment

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

map[any]any ?

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Changes lgtm.

Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Given that there already were some quirks with v2, I don't see a reason for not bumping to v3.

@@ -6058,7 +6058,7 @@ func TestAnnotationsAugmentedError(t *testing.T) {
"# scope: rule\n" +
"p := true\n",
expErrorHint: "Hint: on line 4, symbol(s) ['\\u00a0'] immediately following a key/value separator ':' is not a legal yaml space character",
expErrorRow: 5,
expErrorRow: 6, // Should be 5 really, and yaml.v2 reported the error as on line 1, but v3 on line 3..
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird. But these are just hints after all; don't think this is enough to keep us in the lib stone ages.

t.Fatalf("Expected custom.string to be 'yes' but got %v", value)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -575,9 +575,6 @@ gopkg.in/check.v1
# gopkg.in/ini.v1 v1.67.0
## explicit
gopkg.in/ini.v1
# gopkg.in/yaml.v2 v2.4.0
## explicit; go 1.15
gopkg.in/yaml.v2
Copy link
Contributor

Choose a reason for hiding this comment

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

v3 shouldn't be vendored?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, we already had it vendored ..

Some quirks encountered in the upgrade:
- Errors reported by the parser are now zero-indexed
- A few errors seemingly reported on the line after the
  error rather than where it happened
- Also a few tests where the line number reported
  *previously* seemed wrong but now is right
- Something different in how the parser unmarshalled to
  the "raw" schema annotation type we used. Changed to
  use a a map instead of that type alias.. but I'm not
  really sure why that had to be done

Perhaps worth looking into this further. But pushing this
now to start that discussion.

Fixes open-policy-agent#7090

Signed-off-by: Anders Eknert <[email protected]>
@anderseknert anderseknert merged commit 1d51c1d into open-policy-agent:main Oct 6, 2024
28 checks passed
@anderseknert anderseknert deleted the bye-bye-yaml.v2 branch October 6, 2024 01:54
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.

Metadata parser parsing YAML as version 1.1, leading to "yes"/"no" values parsed as booleans
3 participants