| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-13 | |||
| 15:24:58 | cfriesen | andreykurilin: if you don't @mock.patch on the test itself, how do you do stuff like "client_kwargs = mock_client.call_args_list[0][1]" ? | |
| 15:25:31 | andreykurilin | cfriesen: as I mentioned previously, we need to store object transmitted to MockPatch in setUp method | |
| 15:25:59 | cfriesen | andreykurilin: I think I see. will try that | |
| 15:29:10 | cfriesen | andreykurilin: you are a genius. Looks like that'll work. let me respin a new version for review. | |
| 15:29:47 | andreykurilin | cfriesen: If you have time, please submit a patch with removing all redundant @mock.patch there | |
| 15:30:03 | cfriesen | andreykurilin: whoops, spoke too soon. TypeError: '<MagicMock name='mock().api_version' id='69241360'>' should be an instance of '<class 'novaclient.api_versions.APIVersion'>' | |
| 15:30:53 | andreykurilin | cfriesen: in your test, you should do something like `self.mock_client.api_version = api_versions.APIVersion("2.22")`` where api_versions is a module novaclient.api_versions | |
| 15:31:42 | mriedem | _CellProxy | |
| 15:31:42 | cfriesen | andreykurilin: this is what I have now: http://paste.openstack.org/show/612435/ | |
| 15:31:43 | mriedem | oh _CellProxy | |
| 15:31:46 | mriedem | why oh why | |
| 15:33:18 | cfriesen | andreykurilin: and this is what I get when running it: http://paste.openstack.org/show/612437/ | |
| 15:33:47 | mriedem | cfriesen: needs to be an object, | |
| 15:33:49 | mriedem | not a string | |
| 15:34:38 | mriedem | cfriesen: https://github.com/openstack/python-novaclient/blob/master/novaclient/client.py#L59 | |
| 15:34:43 | mriedem | ^ is what you're mocking out ultimately | |
| 15:34:57 | cfriesen | mriedem: what needs to be an object? | |
| 15:34:58 | mriedem | so it needs to be of type novaclient.api_versions.APIVersion | |
| 15:35:09 | mriedem | cfriesen: self.mock_client.api_version = novaclient.API_MAX_VERSION | |
| 15:35:13 | mriedem | that's a string | |
| 15:35:15 | mriedem | needs to be an object | |
| 15:35:22 | mriedem | an APIVersion object | |
| 15:35:26 | mriedem | which is constructed with a string value | |
| 15:36:02 | mriedem | i would probably also use API_MIN_VERSION as the default | |
| 15:36:06 | mriedem | for existing tests | |
| 15:36:44 | cfriesen | yeah, that was my intent. copy/paste error just now | |
| 15:37:25 | mriedem | bauzas: i've got a fun cells v1 question for you | |
| 15:37:34 | mriedem | bauzas: i need to implement __contains__ for the _CellProxy | |
| 15:37:36 | bauzas | mriedem: kaboom | |
| 15:37:37 | mriedem | like https://github.com/openstack/nova/blob/master/nova/cells/utils.py#L64 | |
| 15:37:41 | mriedem | i see that id and host are special cased | |
| 15:37:50 | mriedem | i was just doing this: | |
| 15:37:51 | mriedem | return key in self._obj | |
| 15:37:51 | mriedem | def __contains__(self, key): | |
| 15:37:56 | mriedem | but do i need to special case id and host? | |
| 15:38:11 | mriedem | i don't think so, but wanted to ask | |
| 15:38:37 | bauzas | mriedem: I need to reload the context in mind because it's a 2-yr blame :p | |
| 15:39:11 | mriedem | i think we special case id and host because they are namespaced | |
| 15:39:12 | mriedem | to the cell | |
| 15:39:28 | mriedem | whether they are IN the proxied object is the same either way i think | |
| 15:41:00 | bauzas | mriedem: oh right I remember | |
| 15:41:01 | bauzas | mriedem: now | |
| 15:41:30 | bauzas | AFAIR, we're amending the host value by the cell path | |
| 15:41:43 | bauzas | the same goes with the ID | |
| 15:41:55 | bauzas | something like 'foo' becomes cell1!foo AFAIR | |
| 15:42:13 | bauzas | which was a problem for our object layers | |
| 15:42:20 | bauzas | at least the id | |
| 15:42:48 | bauzas | so, we wrote those ugly patterns to make sure we can just modify both the ID and the host fields without touching the object | |
| 15:43:48 | mriedem | yeah, so i think the answer is my simple __contains__ impl is fine | |
| 15:43:50 | mriedem | for what i need | |
| 15:43:57 | mriedem | which is just checking if a field is set in the wrapped object | |
| 15:44:08 | mriedem | it's a proxy to the ovo base __contains__ | |
| 15:44:26 | bauzas | I'm thinking of your question | |
| 15:44:33 | bauzas | whether we should special case id and host | |
| 15:44:44 | bauzas | I think no | |
| 15:44:50 | bauzas | because we have the properties | |
| 15:46:59 | bauzas | mriedem: ^ | |
| 15:49:31 | bauzas | mriedem: the thing is, I'm thiking of the precedence between __getitem__ and __contains__ | |
| 15:50:33 | bauzas | mriedem: but if you ask : 'id' in mycellproxy | |
| 15:50:39 | mriedem | bauzas: __contains__ is used before __getitem__ | |
| 15:50:43 | bauzas | it should provide True anyway | |
| 15:50:45 | mriedem | which is why i needed to add it | |
| 15:50:59 | mriedem | see http://logs.openstack.org/99/471899/5/check/gate-nova-python27-ubuntu-xenial/7be94a3/testr_results.html.gz | |
| 15:51:12 | mriedem | that was calling __getitem__ with key=0 | |
| 15:51:17 | bauzas | FWIW, the doc is https://docs.python.org/3/reference/datamodel.html | |
| 15:51:21 | mriedem | treating it as a list rather than a map (dict) | |
| 15:51:37 | mriedem | and getattr blows up with TypeError if you're passing a non-string | |
| 15:51:41 | mriedem | __contains__ fixes it | |
| 15:53:05 | bauzas | mriedem: not sure I get why you land with key=0 but okay | |
| 15:53:37 | mriedem | me neither | |
| 15:53:47 | mriedem | anyway, i think it's resolved, | |
| 15:53:51 | mriedem | forget i ever asked about this | |
| 15:54:16 | bauzas | well, okay :) | |
| 15:55:22 | sfinucan | sdague: Could you take a look at this and see if it makes sense? https://review.openstack.org/#/c/473224/ | |
| 15:55:54 | sdague | sfinucan: with the version bump it should be fine | |
| 15:56:31 | sfinucan | sdague: so they should go in together? | |
| 15:57:00 | sfinucan | *in the same patch | |
| 15:57:06 | sdague | yeh | |
| 15:57:24 | sdague | I think there is going to need to be a unit test change at the same time | |
| 15:57:31 | sdague | I can pull them together if you like | |
| 15:57:45 | cfriesen | mriedem: andreykurilin: Still hitting the problem. http://paste.openstack.org/show/612439/ has my setUp() code, the test itself, and the pdb output while running the test. | |
| 15:58:45 | cfriesen | mriedem: andreykurilin: for some reason I can't do pdb in novaclient/v2/shell.py, it just hangs. wonder if that's related to the issue | |
| 15:59:31 | andreykurilin | it is strange, because I fetched your patch, applied changes in setUp method and everything works for me(at least test_default_endpoint_type passes) | |
| 15:59:55 | cfriesen | andreykurilin:did you change it from "flavor-list" to "list"? | |
| 16:00:12 | andreykurilin | ok, let me try | |
| 16:00:18 | cfriesen | in test_default_endpoint_type9) | |
| 16:02:10 | mriedem | alex_xu: sdague: remember how we wanted to do the os-services and os-hypervisors id->uuid API changes in the same microversion, plus a bunch of other cleanups for weird PUT and GET APIs? | |
| 16:02:26 | mriedem | this one https://specs.openstack.org/openstack/nova-specs/specs/pike/approved/service-hyper-uuid-in-api.html | |
| 16:02:28 | openstackgerrit | Sean Dague proposed openstack/python-novaclient master: Fix setting 'global_request_id' in SessionClient https://review.openstack.org/473224 | |
| 16:03:03 | mriedem | turns out that might be a monster change if we're doing it on the same microversion, like the os-services one is pushing 1000 LOC | |
| 16:03:10 | andreykurilin | cfriesen: `self.mock_client.return_value.api_version = api_versions.APIVersion("2.1")` | |
| 16:03:16 | andreykurilin | you need to setup api_version like this | |
| 16:04:07 | andreykurilin | cfriesen: because self.mock_client is just a mock of novaclient.client.Client method and CLI obtains api_version property from the object which that novaclient.client.Client returns | |
| 16:04:22 | sdague | mriedem: because of the tests and friends? | |
| 16:04:31 | mriedem | sdague: and docs yeah | |
| 16:04:32 | mriedem | plus, | |
| 16:04:38 | mriedem | os-hypervisors is going to require changes for paging across cells, | |
| 16:04:43 | mriedem | which i didn't originally anticipate in the spec | |
| 16:04:46 | sdague | ok | |
| 16:05:02 | sdague | is there any way to structure that code for land better? | |