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

Volume.getMigrateStatus should return null by default #10

Conversation

pjdarton
Copy link
Contributor

@pjdarton pjdarton commented Nov 28, 2019

Fix #9
...and hopefully fix Jenkins OpenStack-cloud-plugin bug 277 in the process.

Summary of changes:

  1. The CinderVolume POJO now "just does what it's told" and doesn't second-guess what the MigrationStatus should be.
    This should then fix the issue where the outgoing JSON request updated the migration status when no such change was requested.

  2. Any (external) caller reading this field should be sufficiently null-proofed by Volume.MigrationStatus.fromValue's null-handling functionality where, when turning JSON to Java, a null JSON string is turned into MigrationStatus.NONE.

i.e. the POJO will (now) retain null as null when writing but anyone reading the data back again will read MigrationStatus.NONE.

@pjdarton
Copy link
Contributor Author

I'm no expert on Travis and I struggle to understand exactly what the failure it reported concerns ... but it built fine on my local machine; I'm not convinced that the CI build failure on Travis is a direct result of my changes...

@olivergondza
Copy link
Member

@pjdarton, welcome to openstack4j/openstack4j!

Please, merge the master. I have fixed the travis build a minute ago.

Copy link
Member

@olivergondza olivergondza left a comment

Choose a reason for hiding this comment

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

I agree the change makes perfect sense. Approved as long as this was tested against real OS instance.

@pjdarton
Copy link
Contributor Author

I've not tested it "for real" ... but if you can tell me (or point me at instructions) how to do a build of the openstack Jenkins plugin that uses a custom build of openstack4j then I can test it "for real" like that.

@olivergondza
Copy link
Member

What I guess would be enough is to make sure openstack4j part is working. I use tester groovy script like so:

$ mvn clean install -DskipTests
$ source openrc.sh
$ groovy test.groovy

At the end of the script you add the code verifying the interaction that used to fail now succeeds. (Given the coverage levels, this might be good contribution guideline for future RPs).

--

Re building openstack-cloud plugin: installing openstack4j locally and then bumping the dependency (to the snapshot one) in the plugin's pom it really all it takes.

@pjdarton
Copy link
Contributor Author

I've tested it by embedding my code-changed-openstack4j into the Jenkins openstack plugin (which is where I originally encountered this bug).
That code sets the name and description on the Volume used by the Instance it's created ... except that fails without this fix.

Without this PR (but with Jenkins openstack-cloud-plugin PR278 to make failure non-fatal), the running instance shows (in the Horizon WebUI):
image
and that volume shows:
image
and the exception described in Jenkins openstack-cloud-plugin issue 277 is logged.

With this PR, the running instance shows:
image
and that volume shows:
image
and no such exception is encountered.

i.e. It's now possible to set the name and description in a Cinder volume.

TL;DR: I think it works.

@pjdarton
Copy link
Contributor Author

I'm going to close, wait a bit, then re-open the PR in the hope that this will re-trigger the Travis CI build.

@pjdarton pjdarton closed this Nov 29, 2019
@pjdarton pjdarton reopened this Nov 29, 2019
@pjdarton pjdarton force-pushed the set-volume-name-shouldnt-set-migstatus branch from e282a5f to 01b1492 Compare November 29, 2019 16:28
@pjdarton
Copy link
Contributor Author

I've rebased on master, the CI build is clean, I've manually tested it and it seems good.
It's ready for merge 👍

Given your earlier stated approval, I would do that myself but github tells me "Merging is blocked" & "You’re not authorized to merge this pull request." ... so I guess merely being a member of https://github.com/openstack4j doesn't automatically include commit access 😉

@olivergondza
Copy link
Member

@pjdarton, Thanks. Merging right away.

Re maintainership, you are first on the list to be asked to become a co-maintainer. Let me put together a doc of what does it entails and I will get back to you with a formulated procedure.

@olivergondza olivergondza merged commit b6b6aaa into openstack4j:master Dec 2, 2019
@pjdarton pjdarton deleted the set-volume-name-shouldnt-set-migstatus branch December 2, 2019 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants