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

bump deps #11148

Merged
merged 3 commits into from
Nov 8, 2023
Merged

bump deps #11148

merged 3 commits into from
Nov 8, 2023

Conversation

Copy link
Contributor

github-actions bot commented Nov 2, 2023

I see that you haven't updated any README files. Would it make sense to do so?

@jmank88
Copy link
Contributor Author

jmank88 commented Nov 2, 2023

@nolag it looks like this cbor version bump is breaking some tests 🤦

@fxamacker
Copy link

@nolag it looks like this cbor version bump is breaking some tests 🤦

Hi, I'm the maintainer of the cbor library. I'm sorry the update to v2.5.0 broke some tests, maybe I can help.

cbor library changes are usually introduced as non-default options to prevent compatibility breaks but some exceptions were made in v2.5 and highlighted in ⭐ Notable Changes to Review Before Upgrading in v2.5.0 release notes.

For example, test breakage might be due to improved error-handling in cbor v2.5.0 (e.g. previously missed problem being detected and reported as error).

A small self-contained reproducer could help projects upgrade to v2.5.0 (e.g. https://github.com/go-webauthn/webauthn v0.8.6 listed in this pull request is also using cbor v2.4.0). I can take a look at the reproducer this weekend if one is available.

Please let me know if I can help in any other way to make the upgrade work.

@jmank88
Copy link
Contributor Author

jmank88 commented Nov 2, 2023

It looks like we just had four test cases failing due to:

Make Unmarshal() and Valid() return cbor.ExtraneousDataError (instead of ignoring extraneous data if any remain).

Two were just incidental bytes to truncate, and two others were explicit trailing bytes cases that needed to change their expectation of success.

@jmank88 jmank88 requested review from nolag and a team November 2, 2023 21:46
@jmank88 jmank88 force-pushed the deps branch 2 times, most recently from 3e55583 to 8760b30 Compare November 2, 2023 22:24
nolag
nolag previously approved these changes Nov 3, 2023
Copy link
Contributor

@nolag nolag left a comment

Choose a reason for hiding this comment

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

Thanks, I'll verify what will need to be done in the branch I'm working on and make it work there or work with the cbor dev :).

@nolag nolag self-requested a review November 3, 2023 13:54
Copy link
Contributor

@nolag nolag left a comment

Choose a reason for hiding this comment

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

I have more context now, we can't just update it. The encoding sent from EVM will have the extra trailing characters. We would need to use the length in the array returned (the first word) to trim the cbor before calling the function.

I'm going to downgrade cbor's use to 2.4 the branch that we're using for our work so we don't need to tackle it now, but we (or the functions team) should try to eventually fix this so it can be upgraded.

@fxamacker
Copy link

I have more context now, we can't just update it. The encoding sent from EVM will have the extra trailing characters. We would need to use the length in the array returned (the first word) to trim the cbor before calling the function.

I'm going to downgrade cbor's use to 2.4 the branch that we're using for our work so we don't need to tackle it now, but we (or the functions team) should try to eventually fix this so it can be upgraded.

@nolag UnmarshalFirst() function added in v2.5.0 can be used to detect and handle extraneous bytes without error.

// UnmarshalFirst decodes first CBOR data item and returns remaining bytes.
rest, err = cbor.UnmarshalFirst(b, &v)   // decode []byte b to v

- github.com/go-webauthn/webauthn v0.8.6

- github.com/prometheus/common v0.45.0

- github.com/prometheus/prometheus v0.47.2

- github.com/tidwall/gjson v1.17.0

- gonum.org/v1/gonum v0.14.0
@nolag nolag marked this pull request as ready for review November 3, 2023 14:34
@nolag nolag requested a review from a team as a code owner November 3, 2023 14:34
@nolag nolag self-requested a review November 3, 2023 14:35
@jmank88 jmank88 enabled auto-merge November 3, 2023 14:51
@jmank88 jmank88 requested a review from samsondav November 3, 2023 14:53
@jmank88 jmank88 added this pull request to the merge queue Nov 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 3, 2023
@jmank88 jmank88 added this pull request to the merge queue Nov 3, 2023
@jmank88 jmank88 removed this pull request from the merge queue due to a manual request Nov 3, 2023
@jmank88 jmank88 requested a review from CL-Andrew November 3, 2023 15:23
@cl-sonarqube-production
Copy link

@jmank88 jmank88 added this pull request to the merge queue Nov 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 8, 2023
@jmank88 jmank88 added this pull request to the merge queue Nov 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 8, 2023
@jmank88 jmank88 added this pull request to the merge queue Nov 8, 2023
Merged via the queue into develop with commit 027068e Nov 8, 2023
84 checks passed
@jmank88 jmank88 deleted the deps branch November 8, 2023 17:47
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