Skip to content

Container Infra v1: Use interface for update value#1463

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
jtopjian:containerinfrav1-update-fix
Feb 21, 2019
Merged

Container Infra v1: Use interface for update value#1463
jtopjian merged 1 commit intogophercloud:masterfrom
jtopjian:containerinfrav1-update-fix

Conversation

@jtopjian
Copy link
Copy Markdown
Contributor

This commit changes the Value field of UpdateOpts to be an interface{}
instead of a string. This is to send type-specific values to the API
service.

For #1458

/cc @tghartland would you be able to test this and confirm it's working?

This commit changes the Value field of UpdateOpts to be an interface{}
instead of a string. This is to send type-specific values to the API
service.
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 21, 2019

Build failed.

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 21, 2019

Coverage Status

Coverage remained the same at 76.388% when pulling 6017439 on jtopjian:containerinfrav1-update-fix into d680128 on gophercloud:master.

@jtopjian
Copy link
Copy Markdown
Contributor Author

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 21, 2019

Build failed.

@tghartland
Copy link
Copy Markdown
Contributor

I've tested this, passing a string to UpdateOpts still causes the value to be rejected by the API (as expected), but now passing an int works.

@jtopjian
Copy link
Copy Markdown
Contributor Author

@tghartland Thank you for testing. Just to confirm: this is expected behavior for you? If the value really should be a string, then this change is still valid, right?

@tghartland
Copy link
Copy Markdown
Contributor

https://github.com/openstack/magnum/blob/9323da78199c7ea5e54d6ec155c32ba6e23968de/api-ref/source/parameters.yaml#L424-L430

The node_count parameter is supposed to be an integer. That it accepts a string when the API is run with python2 is probably unintended.

@jtopjian
Copy link
Copy Markdown
Contributor Author

Right, but what I mean is that when updating other attributes besides node_count, this patch is still valid, correct?

@tghartland
Copy link
Copy Markdown
Contributor

Ah I see what you mean now. I'm not even sure what other parameter I would use to test that.
Looking at the update documentation here it seems as though node_count is the only thing that can be updated.
Given that the I still got the old error exactly as it was before the change when giving the value in the new UpdateOpts as a string again, I think it should still be fine.

@jtopjian
Copy link
Copy Markdown
Contributor Author

Sounds good. Thank you for looking into this.

@jtopjian jtopjian merged commit cd411dc into gophercloud:master Feb 21, 2019
tghartland added a commit to tghartland/gophercloud that referenced this pull request Dec 5, 2019
Required for compatibility with Magnum APIs running on Python 3.
See gophercloud#1458
and gophercloud#1463
jtopjian pushed a commit that referenced this pull request Dec 10, 2019
* Add WaitForTimeout to acceptance tools

To be used in situations where a 300 second timeout is
not long enough or otherwise unsuitable.

The WaitFor function is changed to simply call WaitForTimeout
and pass the same 300 second timeout, keeping the same behaviour.

* Set timeout for cluster creation in containerinfra acceptance tests

Keeps the same timeout as previously by default, but can be changed
for cluster templates that require a longer timeout.

* Check cluster creation error in containerinfra CRUD acceptance

* Use integer for replacing node count in containerinfra acceptance tests

Required for compatibility with Magnum APIs running on Python 3.
See #1458
and #1463

* Use overlay2 for template storage driver in contianerinfra acceptance

With the storage driver set to devicemapper the magnum API did not
accept clusters created with the template, as docker_volume_size
was not set. Instead of setting the volume size, overlay2 can be used
with no volume and is the preferred storage driver.
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