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

Update libtelio dependencies #1066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update libtelio dependencies #1066

wants to merge 1 commit into from

Conversation

jjanowsk
Copy link
Collaborator

@jjanowsk jjanowsk commented Jan 17, 2025

☑️ Definition of Done checklist

  • Commit history is clean (requirements)
  • README.md is updated
  • Functionality is covered by unit or integration tests

@jjanowsk jjanowsk requested a review from a team as a code owner January 17, 2025 09:51
Copy link
Contributor

@LukasPukenis LukasPukenis left a comment

Choose a reason for hiding this comment

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

Missing .unreleased entry. Though the PR itself looks OK and the linked pipeline reveals only one failed job due to Windows VM.

One thing that needs to be addressed - pqcrypto-internals update, it fails on our current Mac builders and I've manually reverted it not long ago so you will need to do that or else it will not build for apple with our runners :). This one is blocking

However without it +1

Comment on lines +3173 to +3174
netmask: Ipv4Addr::new(255, 0, 0, 0),
prefixlen: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Judging by the diff, the netmask was incorrect but the unit test was not failing. Is it possible to make it unit-tested as well while we're at it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was not failing because the code does not care about this netmask at all, and there is not reason to test it. If there was a Default implementation for Ifv4Addr we could use it, but there is not, so we need to initialize it with some dummy data

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