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

make introspection.RootDiskType int64 rather than int (#1634) #1988

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

mwhudson
Copy link
Contributor

@mwhudson mwhudson commented Jul 6, 2020

Fixes tests (and functionality I suspect!) on 32-bit systems.

@coveralls
Copy link

coveralls commented Jul 6, 2020

Coverage Status

Coverage remained the same at 79.416% when pulling fd95bfa on mwhudson:issue-1634 into 2921f45 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 6, 2020

Build failed.

go.mod Outdated
@@ -11,3 +11,5 @@ require (
gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect
gopkg.in/yaml.v2 v2.2.7
)

go 1.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed? It's causing the Travis failure. I'll clean this up in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I meant to remove that before pushing, cleaned up now.

@jtopjian
Copy link
Contributor

jtopjian commented Jul 6, 2020

@mwhudson Thank you for looking into this - it's really appreciated.

From your comments in #1634:

I don't know if it would be too much of an abi break, but

Breaking changes are OK as long as they are justified.

github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection.RootDiskType being an "int" vs an "int64" definitely seems like a mistake.

Yes, from the error message given in #1634, I agree. I'd also be inclined to change other int's to int64 in this package while we're at it.

Fixes tests (and functionality I suspect!) on 32-bit systems.
@mwhudson
Copy link
Contributor Author

mwhudson commented Jul 6, 2020

@mwhudson Thank you for looking into this - it's really appreciated.

No worries, thanks for the quick review!

From your comments in #1634:

I don't know if it would be too much of an abi break, but

Breaking changes are OK as long as they are justified.

Fair enough.

github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection.RootDiskType being an "int" vs an "int64" definitely seems like a mistake.

Yes, from the error message given in #1634, I agree. I'd also be inclined to change other int's to int64 in this package while we're at it.

All of them? I can do that easily enough, of course. I did have a quick scan and didn't think any of the others would be problematic but it might be best to be consistent.

@jtopjian
Copy link
Contributor

jtopjian commented Jul 6, 2020

All of them? I can do that easily enough, of course. I did have a quick scan and didn't think any of the others would be problematic but it might be best to be consistent.

Sorry - I may have been too broad in my last message 🙂

I should have said that we should look at changing them where it makes sense, and only in this introspection package. For example, the Memory fields might work better as an int64. I know it's a long shot to have that much memory, but who knows 2-3 years from now. Things like that.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 6, 2020

Build succeeded.

@mwhudson
Copy link
Contributor Author

mwhudson commented Jul 7, 2020

Well OK. Want to merge this, or fix it yourself how ever you like? :)

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@jtopjian jtopjian merged commit 0183d6a into gophercloud:master Jul 7, 2020
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.

None yet

3 participants