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

Use NetAddr::CIDR.create instead of CIDRv{4,6}.new #2507

Merged
merged 1 commit into from Mar 28, 2024

Conversation

aramprice
Copy link
Member

@aramprice aramprice commented Mar 18, 2024

What is this change about?

The sub-classes of CIDR do not define an #initialize method so these end up calling CIDR#initialize which has the following documentation:

This method performs absolutely no error checking, and is meant to be
used only by other internal methods for the sake of the speedier
creation of CIDR objects. [snip]

See: https://github.com/dspinhirne/netaddr-rb/blob/version-1.5.2/lib/cidr.rb#L205-L216

This commit switches to CIDR.create(..., Version: {4,6}) so that the address type is still explicitly set.

Please provide contextual information.

n/a

What tests have you run against this PR?

rake fly:unit
rake fly:integration

How should this change be described in bosh release notes?

[nternal change, none needed]

Does this PR introduce a breaking change?

No.

@aramprice aramprice marked this pull request as ready for review March 19, 2024 18:15
@jpalermo jpalermo requested review from a team, ystros and selzoc and removed request for a team March 21, 2024 16:00
The sub-classes of `CIDR` do not define an `#initialize` method so these
end up calling `CIDR#initialize` which has the following documentation:

> This method performs absolutely no error checking, and is meant to be
> used only by other internal methods for the sake of the speedier
> creation of CIDR objects. [snip]

This commit switches to `CIDR.create(..., Version: {4,6})` so that the
address type is still explicitly set.
Copy link
Contributor

@ystros ystros left a comment

Choose a reason for hiding this comment

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

Somewhat confused why we are using these CIDR objects in the first place when it seems like we're always passing in singular IP addresses into the constructor. But this change from v#.new to .create looks fine.

@aramprice
Copy link
Member Author

aramprice commented Mar 22, 2024

Somewhat confused why we are using these CIDR objects in the first place ...

That is an excellent question. I don't have a good answer though perhaps this is a leftover from the v1 IP management code.

Another curiosity is why we have vendor'd the 1.x line of a gem which got a massive 2.x rewrite and has been removed from rubygems.org when we only use a small bit of its total functionality.

@beyhan beyhan merged commit 570bfe6 into main Mar 28, 2024
4 checks passed
@beyhan beyhan deleted the user-safer-netaddr-methods branch March 28, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants