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.

Sign up for GitHub

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

Jump to bottom

Updated goccy/go-yaml to v.14.1 #1829

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Updated goccy/go-yaml to v.14.1 #1829

merged 3 commits into from
Nov 19, 2024
+63 −62

Conversation

XuechunHou
Copy link
Contributor

@XuechunHou XuechunHou commented Nov 13, 2024 •
edited
Loading

Description

Updated goccy/go-yaml to v.14.1 by executing the command go get -u github.com/goccy/[email protected]

Related issue

b/376658816

How has this been tested?

Checklist:

  • Unit tests
    • [] Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

@XuechunHou XuechunHou force-pushed the xuechun-go-yaml branch 4 times, most recently from 2098039 to 11063ed Compare November 13, 2024 22:44
braydonk
braydonk reviewed Nov 13, 2024
View reviewed changes
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Looks like there is a breaking change in the pretty printing that this PR caught. I'm going to open an issue upstream about this. Our update can be on hold until then.

@braydonk
Copy link
Contributor

Opened goccy/go-yaml#534

@braydonk
Copy link
Contributor

braydonk commented Nov 14, 2024 •
edited
Loading

@XuechunHou The upstream issue has been resolved. Can you update the package to v1.14.1 and update the goldens?

@XuechunHou XuechunHou changed the title Updated goccy/go-yaml to v.13.1 Updated goccy/go-yaml to v.14.1 Nov 14, 2024
@XuechunHou XuechunHou added the kokoro:force-run Forces kokoro to run integration tests on a CL label Nov 14, 2024
@stackdriver-instrumentation-release stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Nov 14, 2024
braydonk and others added 2 commits November 16, 2024 05:13
@braydonk
Do not use tab character in replacements
5597496 
The tab character is illegal outside of quoted strings in YAML. The go
yaml library used to allow us to do this illegal thing, but not anymore.
@XuechunHou
Merge branch 'master' into xuechun-go-yaml
4d2511d
@XuechunHou XuechunHou added the kokoro:force-run Forces kokoro to run integration tests on a CL label Nov 18, 2024
@stackdriver-instrumentation-release stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Nov 18, 2024
@franciscovalentecastro franciscovalentecastro added the kokoro:force-run Forces kokoro to run integration tests on a CL label Nov 19, 2024
@stackdriver-instrumentation-release stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Nov 19, 2024
franciscovalentecastro
franciscovalentecastro reviewed Nov 19, 2024
View reviewed changes
integration_test/metadata/integration_metadata.go
@@ -155,7 +155,7 @@ func UnmarshalAndValidate(fullPath string, data []byte, i interface{}) error {
v := NewIntegrationMetadataValidator()
// Note: Unmarshaler does not protect when only the key being declared.
// https://github.com/goccy/go-yaml/issues/313
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this comment if this issue (# 313) has been fixed with the current update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't be fixed until 1.14.3, which is available if we want to update to that.

franciscovalentecastro
franciscovalentecastro approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@franciscovalentecastro franciscovalentecastro left a comment

Choose a reason for hiding this comment

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

LGTM modulo the integration test pass and addressing comments.

franciscovalentecastro
franciscovalentecastro reviewed Nov 19, 2024
View reviewed changes
transformation_test/transformation_test.go
@@ -569,7 +569,7 @@ func sanitizeWriteLogEntriesRequest(t *testing.T, r *logpb.WriteLogEntriesReques
func sanitizeStacktrace(t *testing.T, input string) string {
// We need to remove non-deterministic information from stacktraces so the goldens don't keep changing.
// Remove $GOPATH
result := regexp.MustCompile(`(?m)^\t(.*?)pkg/mod/`).ReplaceAllString(input, "\t")
result := regexp.MustCompile(`(?m)^\t(.*?)pkg/mod/`).ReplaceAllString(input, " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why did it has to change from tabs (\t) to two spaces ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using \t (tab) where we did is illegal according to the YAML spec. Tabs can't be used as indentation, and putting the \t character where we did ended up mixing space and tab indentation. The old version of the YAML library parsed this incorrectly, allowing it to be possible when it should be illegal. This is fixed in this newer version of the library. I decided on two spaces to maintain the original intent of the replacement while still making it legal YAML.

@XuechunHou
Copy link
Contributor Author

LGTM modulo the integration test pass and addressing comments.

Ops Agent and Third party integration tests are failing, though they are already failing at Master head. Also, these failed tests on github are passing on Louhi.

These tests mostly failed because "context deadline exceeded"

@XuechunHou XuechunHou merged commit 19a24c5 into master Nov 19, 2024
57 of 69 checks passed
@XuechunHou XuechunHou deleted the xuechun-go-yaml branch November 19, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@braydonk braydonk braydonk left review comments

@franciscovalentecastro franciscovalentecastro franciscovalentecastro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@XuechunHou @braydonk @franciscovalentecastro @stackdriver-instrumentation-release