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

Return the SSID as a string in the legacy API #518

Merged
merged 1 commit into from Aug 3, 2022

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Aug 2, 2022

python-cicoclient expects it to be a string. Sending it as an integer causes comparisons to fail because 505 != '505'.

See CentOS/python-cicoclient#36 for details.

I did not consider too many things since I'm unfamiliar with the codebase. For example, I don't know if this can return None (str(None) fails). Given how cico works I'd expect it to always have SSIDs, but review carefully.

@nphilipp nphilipp self-assigned this Aug 3, 2022
Copy link
Collaborator

@nphilipp nphilipp left a comment

Choose a reason for hiding this comment

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

Hi, thanks for you contribution! The session ID can never be None so casting it to a string should always work, but would you please update the tests/test_legacy.py::TestMain::test_get_nodes test accordingly? It fails at the moment (because it expects the metaclient to return an int here – "15" != 15 😉 – see any of the failed runs:

...
262         if "incorrect" not in testcase and "failure" not in testcase:
263             assert response.status_code == HTTP_200_OK
264             result = response.json()
265             assert isinstance(result, list)
266             assert len(result) == 2
267             assert all(
268                 self.noderesult_to_dict(node, long_form)["hostname"] in ("boo", "bah")
269                 for node in result
270             )
271             if long_form:
272 >               assert all(self.noderesult_to_dict(node, True)["comment"] == 15 for node in result)
273 E               assert False
274 E                +  where False = all(<generator object TestMain.test_get_nodes.<locals>.<genexpr> at 0x7f5980e233c0>)
275
276 tests/test_legacy.py:468: AssertionError
...
342 =========================== short test summary info ============================
343 FAILED tests/test_legacy.py::TestMain::test_get_nodes[unauth] - assert False
344 =================== 1 failed, 543 passed in 79.83s (0:01:19) ===================
...

@ekohl ekohl force-pushed the return-inventory-ssid-as-str branch from 7b1cd85 to 95be9d5 Compare August 3, 2022 08:49
python-cicoclient expects it to be a string. Sending it as an integer
causes comparisons to fail because 505 != '505'.

See CentOS/python-cicoclient#36 for details.
@ekohl ekohl force-pushed the return-inventory-ssid-as-str branch from 95be9d5 to c83ba04 Compare August 3, 2022 08:51
@ekohl
Copy link
Contributor Author

ekohl commented Aug 3, 2022

Updated. Also ran black to format it.

Copy link
Collaborator

@nphilipp nphilipp left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@nphilipp nphilipp merged commit b75bd4d into CentOS:dev Aug 3, 2022
@ekohl ekohl deleted the return-inventory-ssid-as-str branch August 3, 2022 14:53
@ekohl
Copy link
Contributor Author

ekohl commented Aug 3, 2022

I see a new release was cut. Does that mean it should be live in the production API?

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

2 participants