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 gomod #276

Merged
merged 5 commits into from
Apr 11, 2024
Merged

Update gomod #276

merged 5 commits into from
Apr 11, 2024

Conversation

MggMuggins
Copy link
Contributor

I chose to cast all the parameters of util.CanonicalNetworkAddress to the larger int64 rather than converting all port variables to int64 (as in canonical/lxd#13078) as it prevents needing to cast to a smaller int when calling into mdns. That said I'm not attached to one approach or another.

@MggMuggins MggMuggins marked this pull request as ready for review April 3, 2024 21:00
@MggMuggins MggMuggins mentioned this pull request Apr 3, 2024
roosterfish
roosterfish previously approved these changes Apr 4, 2024
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

LGTM!

However I am wondering if there could ever be a port number bigger than the 16 bit unsigned int? Is it that just on some systems the underlying data type is different?
If so wouldn't it be more straightforward to use int64 as type for the port number and convert this number to an int only for the mDNS library?
We could do some checks against math.MaxInt for the upper limit before we try to convert the port number.

@tomponline
Copy link
Member

I would tend to agree with this idea of only converting where you need it.

@MggMuggins
Copy link
Contributor Author

To clarify @tomponline, you would prefer that int64 be used everywhere in microcloud as opposed to int for ports?

@tomponline
Copy link
Member

To clarify @tomponline, you would prefer that int64 be used everywhere in microcloud as opposed to int for ports?

Yeah I think that makes sense - as it avoids the variable size of int on non-64 bit platforms (even though in practice it'll most commonly be on 64 bit platforms).

microcloud/service/service_handler.go Outdated Show resolved Hide resolved
microcloud/go.mod Show resolved Hide resolved
Fixes the build and follows the precedent set in canonical/lxd#13078.

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks :)

Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Thanks, looking good!

@MggMuggins MggMuggins merged commit a28d437 into canonical:main Apr 11, 2024
9 checks passed
@MggMuggins MggMuggins deleted the update-gomod branch April 11, 2024 18:19
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