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

WIP ContainerInfra: extract ID from cluster resize result #1649

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

tghartland
Copy link
Contributor

The cluster resize result should contain just the cluster's ID, so it needs its own Extract method. Currently magnum is sending an empty response which is causing a different error in gophercloud, this will be fixed in magnum with [1].

Will keep as WIP until the fix is merged in magnum and I have been able to test with it.

For #1631

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

[1] https://review.opendev.org/#/c/672600/

@coveralls
Copy link

coveralls commented Jul 25, 2019

Coverage Status

Coverage increased (+0.007%) to 76.997% when pulling 5e40600 on tghartland:magnum-resize-fix-1631 into c99da27 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 25, 2019

Build succeeded.

@jtopjian
Copy link
Contributor

recheck stable/newton

@jtopjian
Copy link
Contributor

oops - wrong PR. Ignore those test results :)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 25, 2019

Build failed.

@jtopjian
Copy link
Contributor

@tghartland It looks like the upstream patch has been merged - is it ready for review? If so, can you confirm that the UUID is the only field being returned now (you're removing the node_count field in the test fixtures).

@tghartland
Copy link
Contributor Author

@jtopjian Yes the fix is merged upstream, I wanted to wait until we had it deployed here so I could test this change and be 100% certain it works (but it's taking longer than expected to get the magnum fix deployed).

This can be reviewed now and I can test later.

Since this changes the results API for extracting the ResizeResult, is there anything special that has to be done or should it just be documented whenever the next release is made?

@jtopjian
Copy link
Contributor

jtopjian commented Sep 3, 2019

@tghartland I'm totally fine waiting until you're able to confirm functionality. We can then also verify if the response body has changed.

If the response has changed, we might want to change how the new extraction is done, similar to what's being done here.

@tghartland
Copy link
Contributor Author

@jtopjian the response should just be the UUID of the cluster

{
   "uuid":"746e779a-751a-456b-a3e9-c883d734946f"
}

The code in this PR should handle that already, is the code you linked doing something else?

@jtopjian
Copy link
Contributor

jtopjian commented Sep 3, 2019

Ah, yeah, I see what's going on.

Prior to this PR, the result was being extracted into a generic Cluster struct. This PR implements a more fine-tuned extraction of just the UUID. So it's quite possible that node_count was never returned in the first place.

Since the results of the extraction method are being changed, we'll want to document it - I'll do that in the changelog.

But again, this can all wait until you've confirmed functionality.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 9, 2019

Build failed.

@tghartland
Copy link
Contributor Author

@jtopjian sorry it's been so long with no activity here. I've now tested this on the magnum master branch and it does work. (The fix has been released in magnum Train now as well).

@jtopjian
Copy link
Contributor

@tghartland No problem at all. I'm going to go ahead and merge this. Since Magnum isn't being actively tested in OpenLab, the test results won't really matter.

Thank you for your work on this.

@jtopjian jtopjian merged commit 433bd8d into gophercloud:master Dec 10, 2019
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.

3 participants